CORS domain config api and config page #4073

TheAndrewJackson · 2023-09-12T17:39:40Z

Closes fidesplus#1065

Description Of Changes

Add CORS configuration to the admin ui and config proxy api.

Screen.Recording.2023-09-14.at.09.38.37.mov

Code Changes

Add CORS configuration page
Update config proxy to have the cors_origin security field
Update config api to update cors_origins within the CORS middleware at runtime

Steps to Confirm

run nox -s dev
update this line to const plus = true; so the admin ui thinks plus is running
run the admin ui
go to /management/cors-configuration
add a new domain(s) to the form and save
restart the server and ensure the values were saved properly
remove the new domain(s) and save

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

cypress · 2023-09-12T17:53:02Z

Passing run #4279 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `ab261c4` into `ce33cf6`...
Project: fides	Commit: `3f77db6504 ℹ️`
Status: Passed	Duration: 01:08 💡
Started: Sep 22, 2023 6:35 PM	Ended: Sep 22, 2023 6:36 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

codecov · 2023-09-12T18:35:24Z

Codecov Report

Patch coverage: 89.83% and project coverage change: -0.01% ⚠️

Comparison is base (37c32f8) 87.51% compared to head (ab261c4) 87.50%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4073      +/-   ##
==========================================
- Coverage   87.51%   87.50%   -0.01%     
==========================================
  Files         326      326              
  Lines       20283    20324      +41     
  Branches     2634     2640       +6     
==========================================
+ Hits        17750    17785      +35     
- Misses       2076     2080       +4     
- Partials      457      459       +2

Files Changed	Coverage Δ
src/fides/api/app_setup.py	`68.91% <63.63%> (-0.87%)`	⬇️
src/fides/api/models/application_config.py	`97.46% <80.00%> (-2.54%)`	⬇️
src/fides/api/api/v1/endpoints/config_endpoints.py	`100.00% <100.00%> (ø)`
src/fides/api/main.py	`76.97% <100.00%> (ø)`
src/fides/api/schemas/application_config.py	`100.00% <100.00%> (ø)`
src/fides/config/config_proxy.py	`100.00% <100.00%> (ø)`
src/fides/config/security_settings.py	`98.93% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

allisonking

nice job, this seems to work great! I left a few minor comments. I'll leave the backend review to Adam :)

clients/admin-ui/src/features/privacy-requests/privacy-requests.slice.ts

clients/admin-ui/src/pages/management/cors-configuration.tsx

TheAndrewJackson · 2023-09-18T17:50:07Z

@adamsachs this should be good to go for another pass 👍

adamsachs

@TheAndrewJackson the code re-org looks great! i think having that sort of side-effect function on the ConfigProxy could be a nice pattern generally.

i just have one concern now on the particular update logic for the cors origins property - i should have realized this before i'd suggested this logic on my previous review 🤦

let me know if it's clear from my comments, i realize things are a bit tricky here and i would be more than happy to talk thru it, since it may be more efficient and i don't wanna hold you up on getting this across the line!

src/fides/api/alembic/migrations/versions/f17f92237383_disable_inactive_notices.py

src/fides/api/api/v1/endpoints/config_endpoints.py

src/fides/config/config_proxy.py

adamsachs · 2023-09-19T12:39:39Z

src/fides/config/config_proxy.py

+            if mw.cls is CORSMiddleware:
+                current_middleware_domains = (
+                    mw.options["allow_origins"]
+                    if mw.options["allow_origins"] is not None
+                    else []
+                )
+                current_config_proxy_domains = (
+                    self.security.cors_origins
+                    if self.security.cors_origins is not None
+                    else []
+                )
+                patch_domains = set(
+                    [*current_middleware_domains, *current_config_proxy_domains]
+                )
+
+                mw.options["allow_origins"] = [str(domain) for domain in patch_domains]


i know i sorta pointed you toward this implementation on your previous iteration, but i hadn't thought of a consequence of this additive logic that now seems like it could be problematic, unless i'm thinking about this wrong.

with the way this is written, won't we never be able to remove a domain that's been added at some point via config setting? initially a domain will be added to the allow_origins list/set, and then wouldn't it essentially stay there forever, since it'd continue to be retrieved as a current_middleware_domains on all subsequent updates (until the service is restarted).

would there be a significant downside to switching the logic here to replace the existing allow_origins list, so that it's exactly what's been specified in the ConfigProxy? i'm not sure if there's an initial set of allow_origins that we need to keep in place from the get-go...if so, i think this problem could be a bit tricky to solve. if not, then i feel like we'd be better off just replacing the cors_origins with whatever's been specified in the ConfigProxy.

i guess ultimately i'm wondering on the motivation for making this additive rather than just a replace. it seems to me like the additive logic is a bit at-odds with the way we set our config properties generally, which assumes a replace sort of functionality. i get worried that trying to reconcile those two can be a bit tricky.

sorry i hadn't thought of this on the previous iteration! but just wanna make sure we get this right 👍

Suggested change

if mw.cls is CORSMiddleware:

current_middleware_domains = (

mw.options["allow_origins"]

if mw.options["allow_origins"] is not None

else []

)

current_config_proxy_domains = (

self.security.cors_origins

if self.security.cors_origins is not None

else []

)

patch_domains = set(

[*current_middleware_domains, *current_config_proxy_domains]

)

mw.options["allow_origins"] = [str(domain) for domain in patch_domains]

if mw.cls is CORSMiddleware:

current_config_proxy_domains = (

self.security.cors_origins

if self.security.cors_origins is not None

else []

)

mw.options["allow_origins"] = [str(domain) for domain in current_config_proxy_domains]

^ may not be exactly how you'd wanna code it, but just to illustrate what i think would lead to a bit more consistent and transparent behavior, if we can get away with it.

adamsachs · 2023-09-19T12:42:16Z

tests/ops/api/v1/endpoints/test_config_endpoints.py

+        # domains and when the `api_set` is reset the middleware
+        # will still have the `config_set` domains loaded in.


right so this is the behavior that i think would be desirable, but per my above comment i'm worried that this isn't actually exactly what's happening. happy to talk through this more because it gets a bit tricky (and perhaps counterintuitive) with the config proxy, but i actually think that if we're going for this behavior, we can accomplish it with what i suggested above 👍

TheAndrewJackson · 2023-09-22T14:55:13Z

src/fides/api/api/v1/endpoints/config_endpoints.py

        merge_updates=False,
    )
+
+    ConfigProxy(db).load_current_cors_domains_into_middleware(request.app)


Nice catch!

adamsachs

BE is looking nice after all the iterations 😄

i have a sneaking suspicion we'll be coming back to the /config API again sometime soon since i feel like edge cases abound (not the concern of this PR), but i feel comfortable this is serving our current purposes well 👍

thanks for all the back and forth on this, i think we landed in a nice spot and are pushing some more generic functionality ⏩

adamsachs added 2 commits September 12, 2023 10:28

initial cut at adding cors-related security settings to config proxy

e190ce7

fix: do not expose cors_origin_regex via api

ff0c4c1

TheAndrewJackson added 7 commits September 12, 2023 16:00

format

4a7a82c

isort

17d3942

pylint

d3207c4

remove comment

83daf2e

Add initial cors config page/form

7a737a8

format and lint

9abdbf4

Fix middleware update

49e284c

TheAndrewJackson self-assigned this Sep 13, 2023

TheAndrewJackson added 8 commits September 13, 2023 17:54

Merge branch 'main' into cors-domain-config-api

aa25bc5

fix lints

7604760

add new types

3563e0c

update types

59f473f

add unit tests

e683ed4

Merge branch 'main' into cors-domain-config-api

d6fed58

update changelog

5abd917

remove assert

4aac46e

TheAndrewJackson requested review from adamsachs and allisonking September 14, 2023 13:40

TheAndrewJackson marked this pull request as ready for review September 14, 2023 13:40

TheAndrewJackson changed the title ~~Cors domain config api~~ CORS domain config api and config page Sep 14, 2023

allisonking reviewed Sep 14, 2023

View reviewed changes

TheAndrewJackson added 4 commits September 14, 2023 12:11

improve typing and naming

3beffee

improve page loading

04aa6e1

use docslink

97a3893

fix lints

d2638dc

TheAndrewJackson requested a review from allisonking September 14, 2023 17:31

TheAndrewJackson added 8 commits September 15, 2023 15:20

Merge branch 'main' into cors-domain-config-api

9bc91bc

DI app instance

2b8b7f1

move domain loading onto ConfigProxy

ccbbfbc

move CORS loading into db startup

6a0c31d

Add docstring and remove resetDomains

bd4cd1f

load set of current and new domains

665a653

fix pylint issues

b29ec91

Fix test failures

f72b040

TheAndrewJackson requested a review from adamsachs September 18, 2023 17:48

adamsachs reviewed Sep 19, 2023

View reviewed changes

TheAndrewJackson and others added 6 commits September 20, 2023 16:17

Update middleware update logic

ca3178a

PUT /config endpoint for cors domain resetting (#4129)

e24460d

Update to use PUT and reset field if empty

0d7fd1e

ensure cors domains are also updated on PUT

b2476d7

update some logging and error messages

27d558b

add test coverage for PUT /config route

0ede2be

TheAndrewJackson commented Sep 22, 2023

View reviewed changes

TheAndrewJackson added 8 commits September 22, 2023 11:06

Update docs link

cae9a3c

Merge branch 'main' into cors-domain-config-api

6116d72

fix mypy issue

88cc943

fix pylint issue

0c1a111

Update casing

e68a354

change type to str

c7edf30

fix test failures

0fa8e7a

update casing... again

ab261c4

adamsachs approved these changes Sep 22, 2023

View reviewed changes

TheAndrewJackson merged commit 72e8346 into main Sep 22, 2023
44 of 45 checks passed

TheAndrewJackson deleted the cors-domain-config-api branch September 22, 2023 20:22

RobertKeyser mentioned this pull request Oct 17, 2023

Wildcard CORS Origin Cannot be Passed in via Environment Variable #4292

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORS domain config api and config page #4073

CORS domain config api and config page #4073

TheAndrewJackson commented Sep 12, 2023 •

edited

Loading

cypress bot commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

allisonking left a comment

TheAndrewJackson commented Sep 18, 2023

adamsachs left a comment

adamsachs Sep 19, 2023

adamsachs Sep 19, 2023

TheAndrewJackson Sep 22, 2023

adamsachs left a comment

		# domains and when the `api_set` is reset the middleware
		# will still have the `config_set` domains loaded in.

CORS domain config api and config page #4073

CORS domain config api and config page #4073

Conversation

TheAndrewJackson commented Sep 12, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

cypress bot commented Sep 12, 2023 • edited Loading

Passing run #4279 ↗︎

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

allisonking left a comment

Choose a reason for hiding this comment

TheAndrewJackson commented Sep 18, 2023

adamsachs left a comment

Choose a reason for hiding this comment

adamsachs Sep 19, 2023

Choose a reason for hiding this comment

adamsachs Sep 19, 2023

Choose a reason for hiding this comment

TheAndrewJackson Sep 22, 2023

Choose a reason for hiding this comment

adamsachs left a comment

Choose a reason for hiding this comment

TheAndrewJackson commented Sep 12, 2023 •

edited

Loading

cypress bot commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading