fides icon indicating copy to clipboard operation
fides copied to clipboard

Prevent modifying default taxonomy fields

Open allisonking opened this issue 3 years ago • 1 comments

Closes https://github.com/ethyca/fides/issues/986

Code Changes

  • [ ] list your code changes here

Steps to Confirm

  • [ ] list any manual steps taken to confirm the changes

Pre-Merge Checklist

  • [ ] All CI Pipelines Succeeded
  • Documentation Updated:
    • [ ] documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • [ ] documentation issue created (tag docs-team to complete issue separately)
  • [ ] Issue Requirements are Met
  • [ ] Relevant Follow-Up Issues Created
  • [ ] Update CHANGELOG.md

Description Of Changes

Write some things here about the changes and any potential caveats

allisonking avatar Aug 15 '22 19:08 allisonking

@ThomasLaPiana would love to get your thoughts on if this is the right track. I think there are two things that need to be covered:

  1. Making sure no edits occur on fields that are default (will this have an undesired effect on the load_default_taxonomy upsert though? i.e. if we make a change in the future, we may not be able to actually apply it without a workaround)
  2. Making sure nothing can edit the is_default field (the db will automatically fill it in as False). Thinking about this now, this will probably make load_default_taxonomy not work since it's trying to insert everything as is_default=True... so maybe this isn't the right way to go

allisonking avatar Aug 15 '22 19:08 allisonking

@allisonking left some comments, lmk if those make sense and seem practical

ThomasLaPiana avatar Aug 16 '22 05:08 ThomasLaPiana

There's a failing test in CI that I'm not sure how to fix. It doesn't fail locally which makes it difficult to debug... but it seems to come down to that in CI the resources_dict somehow sets is_default=True even though it should default to False when there isn't a value given. This causes a 403 which is expected when is_default=True and trying to create via the API, but in this case, it shouldn't be is_default=True in the first place, so I'm not sure what's going on.

https://github.com/ethyca/fides/runs/7882107794?check_suite_focus=true#step:6:232

allisonking avatar Aug 17 '22 15:08 allisonking

There's a failing test in CI that I'm not sure how to fix. It doesn't fail locally which makes it difficult to debug... but it seems to come down to that in CI the resources_dict somehow sets is_default=True even though it should default to False when there isn't a value given. This causes a 403 which is expected when is_default=True and trying to create via the API, but in this case, it shouldn't be is_default=True in the first place, so I'm not sure what's going on.

https://github.com/ethyca/fides/runs/7882107794?check_suite_focus=true#step:6:232

Eduardo figured it out, it was an issue with the pytest fixture scope for resources_dict! Changing the scope to function fixes things nicely.

allisonking avatar Aug 17 '22 17:08 allisonking

This PR may need to be reworked pending updated requirements from @mfbrown . The feature:

Adds a check to put, upsert, and delete which prevents those endpoints from taking any action on models with an is_default=True property. If the attempt is made, throws a 403 forbidden.

is likely not what we want, as it turns out some fields of default taxonomies should be able to be edited.

allisonking avatar Aug 18 '22 13:08 allisonking

Okay, updated the initial comment for the new behavior, should be ready for review again 👍

allisonking avatar Aug 23 '22 21:08 allisonking