diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d26da8d56..ce7f638222 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,8 @@ The types of changes are: - Updating return value for crud.get_custom_fields_filtered [#4575](https://github.com/ethyca/fides/pull/4575) - Updated user deletion confirmation flow to only require one confirmatory input [#4402](https://github.com/ethyca/fides/pull/4402) - Moved `pymssl` to an optional dependency no longer installed by default with our python package [#4581](https://github.com/ethyca/fides/pull/4581) - +- Fixed CORS domain update functionality [#4570](https://github.com/ethyca/fides/pull/4570) +- Update Domains page with ability to add/remove "organization" domains, view "administrator" domains set via security settings, and improve various UX bugs and copy [#4584](https://github.com/ethyca/fides/pull/4584) ### Fixed diff --git a/clients/admin-ui/cypress/e2e/domains.cy.ts b/clients/admin-ui/cypress/e2e/domains.cy.ts new file mode 100644 index 0000000000..51da75b4d5 --- /dev/null +++ b/clients/admin-ui/cypress/e2e/domains.cy.ts @@ -0,0 +1,243 @@ +import { stubPlus } from "cypress/support/stubs"; + +// Mock response for GET /api/v1/config?api_set=true +const API_SET_CONFIG = { + security: { + cors_origins: ["https://example.com", "https://app.example.com"], + }, +}; + +// Mock response for GET /api/v1/config?api_set=false +const CONFIG_SET_CONFIG = { + security: { + cors_origins: ["http://localhost"], + cors_origin_regex: "https://.*\\.example\\.com", + }, +}; + +describe("Domains page", () => { + beforeEach(() => { + cy.login(); + stubPlus(true); + }); + + it("can navigate to the Domains page", () => { + cy.visit("/"); + cy.getByTestId("Domains-nav-link").click(); + cy.getByTestId("management-domains"); + }); + + describe("can view domains", () => { + describe("when existing domains are configured (both api-set and config-set)", () => { + beforeEach(() => { + cy.intercept("GET", "/api/v1/config?api_set=true", API_SET_CONFIG).as( + "getApiSetConfig" + ); + cy.intercept( + "GET", + "/api/v1/config?api_set=false", + CONFIG_SET_CONFIG + ).as("getConfigSetConfig"); + cy.visit("/management/domains"); + }); + + it("can display a loading state while fetching domain configuration", () => { + cy.getByTestId("api-set-domains-form").within(() => { + cy.get(".chakra-spinner"); + }); + cy.getByTestId("config-set-domains-form").within(() => { + cy.get(".chakra-spinner"); + }); + + // After fetching, spinners should disappear + cy.wait("@getApiSetConfig"); + cy.wait("@getConfigSetConfig"); + cy.getByTestId("api-set-domains-form").within(() => { + cy.get(".chakra-spinner").should("not.exist"); + }); + cy.getByTestId("config-set-domains-form").within(() => { + cy.get(".chakra-spinner").should("not.exist"); + }); + }); + + it("can view existing domain configuration, with both api-set and config-set values", () => { + cy.wait("@getApiSetConfig"); + cy.wait("@getConfigSetConfig"); + + cy.getByTestId("api-set-domains-form").within(() => { + cy.getByTestId("input-cors_origins[0]").should( + "have.value", + "https://example.com" + ); + }); + + cy.getByTestId("config-set-domains-form").within(() => { + cy.getByTestId("input-config_cors_origins[0]").should( + "have.value", + "http://localhost" + ); + cy.getByTestId("input-config_cors_origin_regex").should( + "have.value", + "https://.*\\.example\\.com" + ); + }); + }); + }); + + describe("when no existing domains are configured", () => { + beforeEach(() => { + cy.intercept("GET", "/api/v1/config?api_set=true", {}).as( + "getApiSetConfig" + ); + cy.intercept("GET", "/api/v1/config?api_set=false", {}).as( + "getConfigSetConfig" + ); + cy.visit("/management/domains"); + }); + + it("can view empty state", () => { + cy.wait("@getApiSetConfig"); + cy.wait("@getConfigSetConfig"); + + cy.getByTestId("api-set-domains-form").within(() => { + cy.getByTestId("input-cors_origins[0]").should("not.exist"); + }); + + cy.getByTestId("config-set-domains-form").within(() => { + cy.getByTestId("input-config_cors_origins[0]").should("not.exist"); + cy.getByTestId("input-config_cors_origin_regex").should("not.exist"); + cy.contains("No advanced domain settings configured."); + }); + }); + }); + }); + + describe("can edit domains", () => { + beforeEach(() => { + cy.intercept("GET", "/api/v1/config?api_set=true", API_SET_CONFIG).as( + "getApiSetConfig" + ); + cy.intercept("GET", "/api/v1/config?api_set=false", CONFIG_SET_CONFIG).as( + "getConfigSetConfig" + ); + cy.visit("/management/domains"); + cy.wait("@getApiSetConfig"); + cy.wait("@getConfigSetConfig"); + }); + + it("can edit an existing domain", () => { + const API_SET_CONFIG_UPDATED = { + security: { + cors_origins: ["https://www.example.com", "https://app.example.com"], + }, + }; + cy.intercept("PUT", "/api/v1/config", { statusCode: 200 }).as( + "putApiSetConfig" + ); + cy.intercept( + "GET", + "/api/v1/config?api_set=true", + API_SET_CONFIG_UPDATED + ).as("getApiSetConfig"); + + // Edit the first domain and save our changes + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("https://www.example.com"); + cy.getByTestId("save-btn").click(); + cy.wait("@putApiSetConfig").then((interception) => { + const { body } = interception.request; + expect(body) + .to.have.nested.property("security.cors_origins") + .to.eql(["https://www.example.com", "https://app.example.com"]); + }); + cy.wait("@getApiSetConfig"); + cy.getByTestId("input-cors_origins[0]").should( + "have.value", + "https://www.example.com" + ); + cy.getByTestId("toast-success-msg"); + }); + + it("can remove an existing domain", () => { + const API_SET_CONFIG_UPDATED = { + security: { + cors_origins: ["https://app.example.com"], + }, + }; + cy.intercept("PUT", "/api/v1/config", { statusCode: 200 }).as( + "putApiSetConfig" + ); + cy.intercept( + "GET", + "/api/v1/config?api_set=true", + API_SET_CONFIG_UPDATED + ).as("getApiSetConfig"); + + // Delete the first domain and save our changes + cy.get("[aria-label='delete-domain']:first").click(); + cy.getByTestId("save-btn").click(); + cy.wait("@putApiSetConfig").then((interception) => { + const { body } = interception.request; + expect(body) + .to.have.nested.property("security.cors_origins") + .to.eql(["https://app.example.com"]); + }); + cy.wait("@getApiSetConfig"); + cy.getByTestId("input-cors_origins[0]").should( + "have.value", + "https://app.example.com" + ); + cy.getByTestId("toast-success-msg"); + }); + + it("can validate domains", () => { + cy.getByTestId("api-set-domains-form").within(() => { + cy.getByTestId("input-cors_origins[0]").clear().type("foo").blur(); + cy.root().should("contain", "Domain must be a valid URL"); + cy.getByTestId("save-btn").should("be.disabled"); + + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("https://*.foo.com") + .blur(); + cy.root().should("contain", "Domain cannot contain a wildcard"); + cy.getByTestId("save-btn").should("be.disabled"); + + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("https://foo.com/blog") + .blur(); + cy.root().should("contain", "Domain cannot contain a path"); + cy.getByTestId("save-btn").should("be.disabled"); + + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("file://example.txt") + .blur(); + cy.root().should("contain", "Domain must be a valid URL"); + cy.getByTestId("save-btn").should("be.disabled"); + + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("http:foo.com") + .blur(); + cy.root().should("contain", "Domain must be a valid URL"); + cy.getByTestId("save-btn").should("be.disabled"); + + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("https://foo.com/") + .blur(); + cy.root().should("contain", "Domain cannot contain a path"); + cy.getByTestId("save-btn").should("be.disabled"); + + cy.getByTestId("input-cors_origins[0]") + .clear() + .type("https://foo.com") + .blur(); + cy.getByTestId("save-btn").should("be.enabled"); + }); + }); + }); +}); diff --git a/clients/admin-ui/src/features/common/Layout.tsx b/clients/admin-ui/src/features/common/Layout.tsx index 74ea0238da..95aeb26613 100644 --- a/clients/admin-ui/src/features/common/Layout.tsx +++ b/clients/admin-ui/src/features/common/Layout.tsx @@ -49,7 +49,7 @@ const Layout = ({ isValidNotificationRoute; return ( - + Fides Admin UI - {title} diff --git a/clients/admin-ui/src/features/common/form/FormSection.tsx b/clients/admin-ui/src/features/common/form/FormSection.tsx index 37144b682f..4dbf6565d1 100644 --- a/clients/admin-ui/src/features/common/form/FormSection.tsx +++ b/clients/admin-ui/src/features/common/form/FormSection.tsx @@ -1,11 +1,15 @@ -import { Box, BoxProps, Heading, Stack } from "@fidesui/react"; +import { Box, BoxProps, Heading, Stack, Text } from "@fidesui/react"; + +import QuestionTooltip from "~/features/common/QuestionTooltip"; const FormSection = ({ title, + tooltip, children, ...props }: { title: string; + tooltip?: string; } & BoxProps) => ( {title} + {tooltip ? ( + + + + ) : undefined} {children} diff --git a/clients/admin-ui/src/features/common/nav/v2/nav-config.test.ts b/clients/admin-ui/src/features/common/nav/v2/nav-config.test.ts index 407d80ed45..bdbccdc152 100644 --- a/clients/admin-ui/src/features/common/nav/v2/nav-config.test.ts +++ b/clients/admin-ui/src/features/common/nav/v2/nav-config.test.ts @@ -158,7 +158,7 @@ describe("configureNavGroups", () => { }); describe("fides cloud", () => { - it("shows domain records page when fides cloud is enabled", () => { + it("shows domain verification page when fides cloud is enabled", () => { const navGroups = configureNavGroups({ config: NAV_CONFIG, userScopes: ALL_SCOPES, @@ -170,11 +170,11 @@ describe("configureNavGroups", () => { expect( navGroups[4].children .map((c) => c.title) - .find((title) => title === "Domain records") - ).toEqual("Domain records"); + .find((title) => title === "Domain verification") + ).toEqual("Domain verification"); }); - it("does not show domain records page when fides cloud is disabled", () => { + it("does not show domain verification page when fides cloud is disabled", () => { const navGroups = configureNavGroups({ config: NAV_CONFIG, userScopes: ALL_SCOPES, @@ -186,13 +186,13 @@ describe("configureNavGroups", () => { expect( navGroups[4].children .map((c) => c.title) - .find((title) => title === "Domain records") + .find((title) => title === "Domain verification") ).toEqual(undefined); }); }); describe("fides plus", () => { - it("shows cors management when plus and scopes are enabled", () => { + it("shows domain management when plus and scopes are enabled", () => { const navGroups = configureNavGroups({ config: NAV_CONFIG, userScopes: [ @@ -210,11 +210,11 @@ describe("configureNavGroups", () => { .find((c) => c.title === "Domains") ).toEqual({ title: "Domains", - path: routes.CORS_CONFIGURATION_ROUTE, + path: routes.DOMAIN_MANAGEMENT_ROUTE, }); }); - it("hide cors management when plus is disabled", () => { + it("hide domain management when plus is disabled", () => { const navGroups = configureNavGroups({ config: NAV_CONFIG, userScopes: [ @@ -229,11 +229,11 @@ describe("configureNavGroups", () => { expect( navGroups[1].children .map((c) => ({ title: c.title, path: c.path })) - .find((c) => c.title === "Manage domains") + .find((c) => c.title === "Domains") ).toEqual(undefined); }); - it("hide cors management when scopes are wrong", () => { + it("hide domain management when scopes are wrong", () => { const navGroups = configureNavGroups({ config: NAV_CONFIG, userScopes: [ @@ -248,7 +248,7 @@ describe("configureNavGroups", () => { expect( navGroups[1]?.children .map((c) => ({ title: c.title, path: c.path })) - .find((c) => c.title === "Manage domains") + .find((c) => c.title === "Domains") ).toEqual(undefined); }); }); diff --git a/clients/admin-ui/src/features/common/nav/v2/nav-config.ts b/clients/admin-ui/src/features/common/nav/v2/nav-config.ts index a62a6ca7ed..9b1612f498 100644 --- a/clients/admin-ui/src/features/common/nav/v2/nav-config.ts +++ b/clients/admin-ui/src/features/common/nav/v2/nav-config.ts @@ -193,7 +193,7 @@ export const NAV_CONFIG: NavConfigGroup[] = [ scopes: [ScopeRegistryEnum.MESSAGING_CREATE_OR_UPDATE], }, { - title: "Domain records", + title: "Domain verification", path: routes.DOMAIN_RECORDS_ROUTE, requiresPlus: true, requiresFidesCloud: true, @@ -201,7 +201,7 @@ export const NAV_CONFIG: NavConfigGroup[] = [ }, { title: "Domains", - path: routes.CORS_CONFIGURATION_ROUTE, + path: routes.DOMAIN_MANAGEMENT_ROUTE, requiresPlus: true, requiresFidesCloud: false, scopes: [ diff --git a/clients/admin-ui/src/features/common/nav/v2/routes.ts b/clients/admin-ui/src/features/common/nav/v2/routes.ts index c015027a93..291a83f9da 100644 --- a/clients/admin-ui/src/features/common/nav/v2/routes.ts +++ b/clients/admin-ui/src/features/common/nav/v2/routes.ts @@ -33,5 +33,5 @@ export const ABOUT_ROUTE = "/management/about"; export const CUSTOM_FIELDS_ROUTE = "/management/custom-fields"; export const EMAIL_TEMPLATES_ROUTE = "/management/email-templates"; export const DOMAIN_RECORDS_ROUTE = "/management/domain-records"; -export const CORS_CONFIGURATION_ROUTE = "/management/cors-configuration"; +export const DOMAIN_MANAGEMENT_ROUTE = "/management/domains"; export const GLOBAL_CONSENT_CONFIG_ROUTE = "/management/consent"; diff --git a/clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts b/clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts index 8cc5b0af66..daa3ebb104 100644 --- a/clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts +++ b/clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts @@ -1,5 +1,4 @@ import { createSelector, createSlice, PayloadAction } from "@reduxjs/toolkit"; -import { narrow } from "narrow-minded"; import { baseApi } from "~/features/common/api.slice"; import { @@ -501,31 +500,52 @@ export const { } = privacyRequestApi; export type CORSOrigins = Pick; -const EMPTY_CORS_DOMAINS: CORSOrigins = { - cors_origins: [], +/** + * NOTE: + * 1. "configSet" stores the results from `/api/v1/config?api_set=false`, and + * contains the config settings that are set exclusively on the server via + * TOML/ENV configuration. + * 2. "apiSet" stores the results from `/api/v1/config?api_set=true`, and + * are the config settings that we can read/write via the API. + * + * These two settings are merged together at runtime by Fides when enforcing + * CORS origins, and although they're awkwardly-named concepts (try saying + * "config set config settings" 10 times fast), we're mirroring the API here to + * be consistent! + */ +export type CORSOriginsSettings = { + // EXTRA NOTE: We also include the "cors_origin_regex" setting here as a read-only config + configSet: Pick< + SecurityApplicationConfig, + "cors_origins" | "cors_origin_regex" + >; + apiSet: Pick; }; -export const selectCORSOrigins = () => + +export const selectCORSOrigins: (state: RootState) => CORSOriginsSettings = createSelector( [ (state) => state, privacyRequestApi.endpoints.getConfigurationSettings.select({ api_set: true, }), + privacyRequestApi.endpoints.getConfigurationSettings.select({ + api_set: false, + }), ], - (_, { data }) => { - const hasCorsOrigins = narrow( - { - security: { - cors_origins: ["string"], - }, + (_, { data: apiSetConfig }, { data: configSetConfig }) => { + // Return a single state contains the current CORS config with both + // config-set and api-set values + const currentCORSOriginSettings: CORSOriginsSettings = { + configSet: { + cors_origins: configSetConfig?.security?.cors_origins || [], + cors_origin_regex: configSetConfig?.security?.cors_origin_regex, + }, + apiSet: { + cors_origins: apiSetConfig?.security?.cors_origins || [], }, - data - ); - - const corsOrigins: CORSOrigins = { - cors_origins: data?.security?.cors_origins, }; - return hasCorsOrigins ? corsOrigins : EMPTY_CORS_DOMAINS; + return currentCORSOriginSettings; } ); diff --git a/clients/admin-ui/src/pages/consent/configure/index.tsx b/clients/admin-ui/src/pages/consent/configure/index.tsx index 63a6d00b27..28622fbf00 100644 --- a/clients/admin-ui/src/pages/consent/configure/index.tsx +++ b/clients/admin-ui/src/pages/consent/configure/index.tsx @@ -1,7 +1,7 @@ import { Box, Flex, Heading, Text } from "@fidesui/react"; import React from "react"; -import FixedLayout from "~/features/common/FixedLayout"; +import Layout from "~/features/common/Layout"; import { ConsentManagementTable } from "~/features/configure-consent/ConsentManagementTable"; type Props = { @@ -25,19 +25,13 @@ const ConsentMetadata: React.FC = ({ title, description }) => ( ); const ConfigureConsentPage = () => ( - + - + ); export default ConfigureConsentPage; diff --git a/clients/admin-ui/src/pages/management/cors-configuration.tsx b/clients/admin-ui/src/pages/management/domains.tsx similarity index 51% rename from clients/admin-ui/src/pages/management/cors-configuration.tsx rename to clients/admin-ui/src/pages/management/domains.tsx index b5b12473f3..ae97bb82ad 100644 --- a/clients/admin-ui/src/pages/management/cors-configuration.tsx +++ b/clients/admin-ui/src/pages/management/domains.tsx @@ -1,5 +1,4 @@ /* eslint-disable react/no-array-index-key */ -import { AddIcon } from "@chakra-ui/icons"; import { Box, Button, @@ -20,7 +19,7 @@ import * as Yup from "yup"; import { useAppSelector } from "~/app/hooks"; import DocsLink from "~/features/common/DocsLink"; import FormSection from "~/features/common/form/FormSection"; -import { CustomTextInput } from "~/features/common/form/inputs"; +import { CustomTextInput, TextInput } from "~/features/common/form/inputs"; import { getErrorMessage, isErrorResult } from "~/features/common/helpers"; import Layout from "~/features/common/Layout"; import { errorToastParams, successToastParams } from "~/features/common/toast"; @@ -39,17 +38,84 @@ const CORSConfigurationPage: NextPage = () => { const { isLoading: isLoadingGetQuery } = useGetConfigurationSettingsQuery({ api_set: true, }); - const corsOrigins = useAppSelector(selectCORSOrigins()); + const { isLoading: isLoadingConfigSetQuery } = + useGetConfigurationSettingsQuery({ + api_set: false, + }); + const currentSettings = useAppSelector(selectCORSOrigins); + const apiSettings = currentSettings.apiSet; + const configSettings = currentSettings.configSet; + const hasConfigSettings: boolean = !!( + configSettings.cors_origins?.length || configSettings.cors_origin_regex + ); const applicationConfig = useAppSelector(selectApplicationConfig()); const [putConfigSettingsTrigger, { isLoading: isLoadingPutMutation }] = usePutConfigurationSettingsMutation(); const toast = useToast(); + const isValidURL = (value: string | undefined) => { + if ( + !value || + !(value.startsWith("https://") || value.startsWith("http://")) + ) { + return false; + } + try { + /* eslint-disable-next-line no-new */ + new URL(value); + } catch (e) { + return false; + } + return true; + }; + + const containsNoWildcard = (value: string | undefined) => { + if (!value) { + return false; + } + return !value.includes("*"); + }; + + const containsNoPath = (value: string | undefined) => { + if (!value) { + return false; + } + try { + const url = new URL(value); + return url.pathname === "/" && !value.endsWith("/"); + } catch (e) { + return false; + } + }; + const ValidationSchema = Yup.object().shape({ cors_origins: Yup.array() .nullable() - .of(Yup.string().required().trim().url().label("URL")), + .of( + Yup.string() + .required() + .trim() + .test( + "is-valid-url", + ({ label }) => + `${label} must be a valid URL (e.g. https://example.com)`, + (value) => isValidURL(value) + ) + .test( + "has-no-wildcard", + ({ label }) => + `${label} cannot contain a wildcard (e.g. https://*.example.com)`, + (value) => containsNoWildcard(value) + ) + .test( + "has-no-path", + ({ label }) => + `${label} cannot contain a path (e.g. https://example.com/path)`, + (value) => containsNoPath(value) + ) + .label("Domain") + ), }); const handleSubmit = async ( @@ -63,11 +129,11 @@ const CORSConfigurationPage: NextPage = () => { if (isErrorResult(result)) { const errorMsg = getErrorMessage( result.error, - `An unexpected error occurred while saving CORS domains. Please try again.` + `An unexpected error occurred while saving domains. Please try again.` ); toast(errorToastParams(errorMsg)); } else { - toast(successToastParams("CORS domains saved successfully")); + toast(successToastParams("Domains saved successfully")); // Reset state such that isDirty will be checked again before next save formikHelpers.resetForm({ values }); } @@ -78,6 +144,7 @@ const CORSConfigurationPage: NextPage = () => { ? values.cors_origins : undefined; + // Ensure that we include the existing applicationConfig (for other API-set configs) const payload: PlusApplicationConfig = { ...applicationConfig, security: { @@ -91,38 +158,41 @@ const CORSConfigurationPage: NextPage = () => { }; return ( - - + + - Manage domains + Domains - - Manage domains for your organization - - - You must add domains associated with your organization to Fides to - ensure features such as consent function correctly. For more - information on managing domains on Fides, click here{" "} - - docs.ethyca.com + + For Fides to work on your website(s), each of your domains must be + listed below. You can add and remove domains at any time up to the + quantity included in your license. For more information on managing + domains{" "} + + read here . - - + + {isLoadingGetQuery || isLoadingPutMutation ? ( ) : ( - initialValues={corsOrigins} + initialValues={apiSettings} enableReinitialize onSubmit={handleSubmit} validationSchema={ValidationSchema} + validateOnChange > {({ dirty, values, isValid }) => (
@@ -136,11 +206,12 @@ const CORSConfigurationPage: NextPage = () => { { @@ -189,6 +260,47 @@ const CORSConfigurationPage: NextPage = () => { )} + + + {isLoadingConfigSetQuery ? ( + + + + ) : ( + + {configSettings.cors_origins!.map((origin, index) => ( + + ))} + {configSettings.cors_origin_regex ? ( + + ) : undefined} + {!hasConfigSettings ? ( + + No advanced domain settings configured. + + ) : undefined} + + )} + + ); diff --git a/clients/admin-ui/src/types/api/models/SecurityApplicationConfig.ts b/clients/admin-ui/src/types/api/models/SecurityApplicationConfig.ts index 009e6a60df..cee7149e83 100644 --- a/clients/admin-ui/src/types/api/models/SecurityApplicationConfig.ts +++ b/clients/admin-ui/src/types/api/models/SecurityApplicationConfig.ts @@ -7,7 +7,11 @@ */ export type SecurityApplicationConfig = { /** - * A list of client addresses allowed to communicate with the Fides webserver. + * A list of HTTP origins allowed to communicate with the Fides webserver. */ cors_origins?: Array; + /** + * An optional regex to allowlist wider patterns of HTTP origins to communicate with the Fides webserver. + */ + cors_origin_regex?: string; }; diff --git a/src/fides/api/models/application_config.py b/src/fides/api/models/application_config.py index 131591aa0d..c17a4e4253 100644 --- a/src/fides/api/models/application_config.py +++ b/src/fides/api/models/application_config.py @@ -1,7 +1,7 @@ from __future__ import annotations from json import loads -from typing import Any, Dict, Optional +from typing import Any, Dict, Iterable, Optional from loguru import logger from pydantic.utils import deep_update @@ -169,6 +169,20 @@ def clear_api_set(cls, db: Session) -> Optional[ApplicationConfig]: return existing_record return None + @classmethod + def clear_config_set(cls, db: Session) -> Optional[ApplicationConfig]: + """ + Utility method to set the `config_set` column on the `applicationconfig` + db record to an empty dict + + """ + existing_record = db.query(cls).first() + if existing_record: + existing_record.config_set = {} + existing_record.save(db) + return existing_record + return None + @classmethod def update_config_set(cls, db: Session, config: FidesConfig) -> ApplicationConfig: """ @@ -184,21 +198,42 @@ def update_config_set(cls, db: Session, config: FidesConfig) -> ApplicationConfi @classmethod def get_resolved_config_property( - cls, db: Session, config_property: str, default_value: Any = None + cls, + db: Session, + config_property: str, + merge_values: bool = False, + default_value: Any = None, ) -> Optional[Any]: """ Gets the 'resolved' config property based on api-set and config-set configs. `config_property` is a dot-separated path to the config property, e.g. `notifications.notification_service_type`. - Api-set values get priority over config-set, in case of conflict. + Api-set values get priority over config-set, in case of conflict, unless `merge_values` + is specified as `True`, in which case the proxy will attempt to _merge_ the api-set + and config-set values. Note that only iterable config values (e.g. lists) can be merged! + + An error will be thrown if `merge_values` is used for a property with non-iterable values. """ config_record = db.query(cls).first() if config_record: api_prop = get(config_record.api_set, config_property) + config_prop = get(config_record.config_set, config_property, default_value) + # if no api-set property found, fall back to config-set if api_prop is None: - logger.debug(f"No API-set {config_property} property found") - return get(config_record.config_set, config_property, default_value) + return config_prop + + # if we want to merge values and have a config property too + if merge_values and config_prop is not None: + # AND we have iterable api and config-set properties, then we try to merge + if not isinstance(api_prop, Iterable) or not isinstance( + config_prop, Iterable + ): + raise RuntimeError("Only iterable values can be merged!") + return {value for value in (list(api_prop) + list(config_prop))} + + # otherwise, we just use the api-set property return api_prop + logger.warning("No config record found!") return default_value diff --git a/src/fides/config/config_proxy.py b/src/fides/config/config_proxy.py index 4952ffc1a3..c21e68302e 100644 --- a/src/fides/config/config_proxy.py +++ b/src/fides/config/config_proxy.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, List, Optional +from typing import Any, Callable, Iterable, List, Optional, Set from fastapi.applications import FastAPI from pydantic import AnyUrl @@ -12,6 +12,19 @@ from fides.config import CONFIG +def merge_properties(attribute_names: Iterable[str]) -> Callable: + """ + Decorator to specify a config proxy class's attributes as config properties + whose api-set and config-set values should be _merged_ when resolved by the proxy. + """ + + def decorator(cls: ConfigProxyBase) -> ConfigProxyBase: + cls.merge_properties = set(attribute_names) + return cls + + return decorator + + class ConfigProxyBase: """ Base class that's used to make config proxy classes that correspond @@ -22,9 +35,34 @@ class ConfigProxyBase: Config proxy classes allow these "resolved" properties to be looked up as if they were a "normal" pydantic config object, i.e. with dot notation, e.g. `ConfigProxy(db).notifications.notification_service_type` + + Merging: to have a given config property's api-set and config-set values to be merged + on property resolution, use the `@merge_properties` decorator on the config proxy + class to specify the corresponding attribute name. + + For example, if creating a config proxy class: + ``` + @merge_properties(["cors_origins"]) + class SecuritySettingsProxy(ConfigProxyBase): + prefix = "security" + cors_origins: List[str] + ``` + + and `security.cors_origins` has the following values: + config-set: `["a", "b"]` + api-set: `["b", "c"]` + + then, when using the config proxy to access the attribute, it will return + a merged set of values: + ``` + >>> ConfigProxy(db).security.cors_origins + {"a", "b", "c"} + ``` + """ prefix: str + merge_properties: Set[str] = set() def __init__(self, db: Session) -> None: self._db = db @@ -35,10 +73,12 @@ def __getattribute__(self, __name: str) -> Any: using the config proxy as a "normal" config object, i.e. with dot notation, e.g. `config_proxy.notifications.notification_service_type """ - if __name in ("_db", "prefix"): + if __name in ("_db", "merge_properties", "prefix"): return object.__getattribute__(self, __name) return ApplicationConfig.get_resolved_config_property( - self._db, f"{self.prefix}.{__name}" + self._db, + f"{self.prefix}.{__name}", + merge_values=__name in self.merge_properties, ) @@ -83,6 +123,7 @@ class StorageSettingsProxy(ConfigProxyBase): active_default_storage_type: StorageType +@merge_properties(["cors_origins"]) class SecuritySettingsProxy(ConfigProxyBase): prefix = "security" @@ -133,9 +174,11 @@ def load_current_cors_domains_into_middleware(self, app: FastAPI) -> None: `ConfigProxy` into the `CORSMiddleware` at runtime. """ + # NOTE: `cors_origins` config proxy resolution _merges_ api-set and config-set values, if both present current_config_proxy_domains = ( self.security.cors_origins if self.security.cors_origins is not None else [] ) + update_cors_middleware( app, current_config_proxy_domains, diff --git a/src/fides/config/utils.py b/src/fides/config/utils.py index c14884f242..4afff5c90a 100644 --- a/src/fides/config/utils.py +++ b/src/fides/config/utils.py @@ -50,6 +50,7 @@ def get_dev_mode() -> bool: ], "security": [ "cors_origins", + "cors_origin_regex", "encoding", "oauth_access_token_expire_minutes", ], diff --git a/tests/ops/api/v1/endpoints/test_config_endpoints.py b/tests/ops/api/v1/endpoints/test_config_endpoints.py index b3ca249df6..da0e3a3beb 100644 --- a/tests/ops/api/v1/endpoints/test_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_config_endpoints.py @@ -330,12 +330,12 @@ def test_patch_application_config_updates_cors_domains_in_middleware( assert set(current_cors_middleware.options["allow_origins"]) == set( payload["security"]["cors_origins"] - ) + ).union(set(original_cors_middleware_origins)) # now ensure that the middleware update was effective by trying # some sample requests with different origins - # first, try with an original allowed origin, which should no longer be accepted + # first, try with an original allowed origin, which should still be accepted assert len(original_cors_middleware_origins) headers = { **auth_header, @@ -343,10 +343,9 @@ def test_patch_application_config_updates_cors_domains_in_middleware( "Access-Control-Request-Method": "PATCH", } response = api_client.options(url, headers=headers) - assert response.status_code == 400 - assert response.text == "Disallowed CORS origin" + assert response.status_code == 200 - # now try with a new allowed origin, which should be accepted + # now try with a new allowed origin, which should also be accepted headers = { **auth_header, "Origin": payload["security"]["cors_origins"][0], @@ -680,6 +679,18 @@ def test_put_application_config_updates_cors_domains_in_middleware( original_cors_middleware_origins, ): auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + new_cors_origins = payload["security"]["cors_origins"] + + # try with a new allowed origin, which should not be accepted yet + # since we have not made the config update yet + headers = { + **auth_header, + "Origin": new_cors_origins[0], + "Access-Control-Request-Method": "PUT", + } + response = api_client.options(url, headers=headers) + assert response.status_code == 400 + assert response.text == "Disallowed CORS origin" # if we set cors origins values via API, ensure that those are the # `allow_origins` values on the active cors middleware @@ -691,16 +702,15 @@ def test_put_application_config_updates_cors_domains_in_middleware( assert response.status_code == 200 current_cors_middleware = find_cors_middleware(api_client.app) - new_cors_origins = payload["security"]["cors_origins"] assert set(current_cors_middleware.options["allow_origins"]) == set( new_cors_origins - ) + ).union(set(original_cors_middleware_origins)) # now ensure that the middleware update was effective by trying # some sample requests with different origins - # first, try with an original allowed origin, which should no longer be accepted + # first, try with an original allowed origin, which should still be accepted assert len(original_cors_middleware_origins) headers = { **auth_header, @@ -708,10 +718,9 @@ def test_put_application_config_updates_cors_domains_in_middleware( "Access-Control-Request-Method": "PUT", } response = api_client.options(url, headers=headers) - assert response.status_code == 400 - assert response.text == "Disallowed CORS origin" + assert response.status_code == 200 - # now try with a new allowed origin, which should be accepted + # now try with a new allowed origin, which should also be accepted headers = { **auth_header, "Origin": new_cors_origins[0], @@ -720,6 +729,16 @@ def test_put_application_config_updates_cors_domains_in_middleware( response = api_client.options(url, headers=headers) assert response.status_code == 200 + # but ensure that an unrelated origin is still not allowed + headers = { + **auth_header, + "Origin": "https://fakesite.com", + "Access-Control-Request-Method": "PUT", + } + response = api_client.options(url, headers=headers) + assert response.status_code == 400 + assert response.text == "Disallowed CORS origin" + # but then ensure that we can revert back to the original values # by PUTing a config that does not have cors origins specified payload.pop("security") # remove cors origins from PUT payload @@ -738,7 +757,7 @@ def test_put_application_config_updates_cors_domains_in_middleware( # now ensure that the middleware update was effective by trying # some sample requests with different origins - # first, try with an original allowed origin, which should now be accepted + # first, try with an original allowed origin, which should still be accepted assert len(original_cors_middleware_origins) headers = { **auth_header, @@ -1106,6 +1125,7 @@ def test_get_config( "encoding", "oauth_access_token_expire_minutes", "subject_request_download_link_ttl_seconds", + "cors_origin_regex", ] ) ) diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index b75de22185..468478a507 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -1,5 +1,5 @@ from json import dumps -from typing import Any, Dict +from typing import Any, Dict, Iterable import pytest from sqlalchemy.orm import Session @@ -274,6 +274,12 @@ def example_config_dict(self) -> Dict[str, str]: "notification_service_type": "twilio_email", "nested_setting_2": "nested_value_2", }, + "security": { + "cors_origins": [ + "http://additionalorigin:8080", + "http://localhost", # this is a _repeated_ value from the config-set values + ] + }, } @pytest.fixture(scope="function") @@ -327,6 +333,86 @@ def test_get_resolved_config_property_no_config_record(self, db: Session): ) assert notification_service_type is None + @pytest.mark.usefixtures("insert_app_config") + def test_get_resolved_config_property_merged_values( + self, db: Session, insert_example_config_record + ): + # ensure our config set has cors origins set as a populated list initially, for a valid test + cors_origins_config_set = ApplicationConfig.get_config_set(db)["security"][ + "cors_origins" + ] + assert cors_origins_config_set + assert isinstance(cors_origins_config_set, Iterable) + # and ensure there are repeats between our API-set list and our config-set list, for a valid test + cors_origin_api_set = insert_example_config_record["security"]["cors_origins"] + repeat_values = set(cors_origin_api_set).intersection( + set(cors_origins_config_set) + ) + assert repeat_values + + # now ensure the merging works as expected on resolution + resolved_origins = ApplicationConfig.get_resolved_config_property( + db, + "security.cors_origins", + merge_values=True, + ) + for config_set_value in cors_origins_config_set: + assert config_set_value in resolved_origins + for api_set_value in cors_origin_api_set: + assert api_set_value in resolved_origins + for resolve_origin in resolved_origins: + assert ( + resolve_origin in cors_origins_config_set + or resolve_origin in cors_origin_api_set + ) + assert isinstance(resolved_origins, set) + + # ensure baseline (non-merging) functionality works as expected + assert sorted( + ApplicationConfig.get_resolved_config_property(db, "security.cors_origins") + ) == sorted(cors_origin_api_set) + + # ensure that merging is not performed if either api-set or config-set values aren't present + insert_example_config_record["security"].pop("cors_origins") + # clear the api-set cors origin + ApplicationConfig.clear_api_set(db) + ApplicationConfig.create_or_update( + db, + data={ + "api_set": insert_example_config_record, + }, + ) + # even if merge is set to true, we should just return config set + assert ( + ApplicationConfig.get_resolved_config_property( + db, + "security.cors_origins", + merge_values=True, + ) + == cors_origins_config_set + ) + # now clear the config-set cors origin + insert_example_config_record["security"]["cors_origins"] = cors_origin_api_set + new_config_set = ApplicationConfig.get_config_set(db) + new_config_set["security"].pop("cors_origins") + ApplicationConfig.clear_config_set(db) + ApplicationConfig.create_or_update( + db, + data={ + "api_set": insert_example_config_record, + "config_set": new_config_set, + }, + ) + # even if merge is set to true, we should just return api set + assert ( + ApplicationConfig.get_resolved_config_property( + db, + "security.cors_origins", + merge_values=True, + ) + == cors_origin_api_set + ) + class TestConfigProxy: @pytest.fixture(scope="function") @@ -338,6 +424,12 @@ def example_config_dict(self) -> Dict[str, str]: "notification_service_type": "twilio_email", "nested_setting_2": "nested_value_2", }, + "security": { + "cors_origins": [ + "http://additionalorigin:8080", + "http://localhost", # this is a _repeated_ value from the config-set values + ] + }, } @pytest.fixture(scope="function") @@ -442,3 +534,35 @@ def test_config_proxy_unset( assert ( notification_service_type == CONFIG.notifications.notification_service_type ) + + @pytest.mark.usefixtures("insert_app_config") + def test_config_proxy_merged_values( + self, db, config_proxy: ConfigProxy, insert_example_config_record + ): + """Ensure config proxy properly merges cors_origin api-set and config-set values""" + + # ensure our config set has cors origins set as a populated list initially, for a valid test + cors_origins_config_set = ApplicationConfig.get_config_set(db)["security"][ + "cors_origins" + ] + assert cors_origins_config_set + assert isinstance(cors_origins_config_set, Iterable) + # and ensure there are repeats between our API-set list and our config-set list, for a valid test + cors_origin_api_set = insert_example_config_record["security"]["cors_origins"] + repeat_values = set(cors_origin_api_set).intersection( + set(cors_origins_config_set) + ) + assert repeat_values + + proxy_resolved_cors_origins = config_proxy.security.cors_origins + + for config_set_value in cors_origins_config_set: + assert config_set_value in proxy_resolved_cors_origins + for api_set_value in cors_origin_api_set: + assert api_set_value in proxy_resolved_cors_origins + for resolve_origin in proxy_resolved_cors_origins: + assert ( + resolve_origin in cors_origins_config_set + or resolve_origin in cors_origin_api_set + ) + assert isinstance(proxy_resolved_cors_origins, set)