Improve API error messages when using `is_default` field on taxonomy resources #5147

NevilleS · 2024-07-31T18:13:30Z

Description Of Changes

Today we throw a generic 403 error for a variety of edits which make it look like the user is lacking sufficient permissions, but in reality it is that they are forbidden from editing the is_default field on a resource (e.g. DataUse, DataCategory).

Current Error Message

For example, we had a user report this unexpected error: they called the /api/v1/data_category/upsert API with this body:

[
    {
        "version_added": "2.0.0",
        "is_default": true,
        "fides_key": "user.unique_id.my_custom_category",
        "organization_fides_key": "default_organization",
        "name": "custom data category",
        "description": "description for custom data category",
        "parent_key": "user.unique_id",
        "active": true
    }
]

Which returns this error:

{ 'detail': { 'error': 'user does not have permission to modify',
              'fides_key': 'user.unique_id.my_custom_category',
              'resource_type': 'DataCategory'}}

New Error Message

As you can see above, the message is confusing - the error actually isn't that the user is lacking permission, it's that the user is trying to create a resource with is_default: true, which we never allow.

With this change, the error looks like this instead:

{ 'detail': { 'error': "cannot create a resource where 'is_default' is true",
              'fides_key': 'user.unique_id.my_custom_category',
              'resource_type': 'DataCategory'}}

There are some slight variations on those messages for deleting/editing/etc.

Code Changes

Added a ForbiddenIsDefaultTaxonomyError error type with more user-friendly error messages
Updated test_api.py integration tests to assert on better error messages
Use the ForbiddenIsDefaultTaxonomyError error in the various code paths that were throwing generic 403s due to is_default rejections

Steps to Confirm

Run integration tests

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- ~~[ ] documentation complete, PR opened in fidesdocs~~
- ~~[ ] documentation issue created in fidesdocs~~
- ~~[ ] if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry~~
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
~~[ ] For API changes, the Postman collection has been updated~~
If there are any database migrations:
- ~~[ ] Ensure that your downrev is up to date with the latest revision on main~~
- ~~[ ] Ensure that your downgrade() migration is correct and works~~
  - ~~[ ] If a downgrade migration is not possible for this change, please call this out in the PR description!~~

…fault field on taxonomy resources

vercel · 2024-07-31T18:13:34Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Jul 31, 2024 6:48pm

cypress · 2024-07-31T18:27:13Z

Test summary

4 • 0 • 0 • 0

Run details


Project	fides
Status	Passed
Commit	b12e8af0ff ℹ️
Started	Jul 31, 2024 6:59 PM
Ended	Jul 31, 2024 7:00 PM
Duration	00:36 💡
OS	Linux Ubuntu -
Browser	Electron 106

View run in Cypress Cloud ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

NevilleS

Quick self-review

NevilleS · 2024-07-31T18:35:09Z

src/fides/api/util/errors.py

        detail = {
-            "error": "user does not have permission to modify",


This was the main issue - this error message couldn't be overridden with anything more relevant to more specific types of 403 errors.

NevilleS · 2024-07-31T18:35:46Z

src/fides/api/util/errors.py

            "resource_type": resource_type,
            "fides_key": fides_key,
        }
        super().__init__(status.HTTP_403_FORBIDDEN, detail=detail)


+class ForbiddenIsDefaultTaxonomyError(ForbiddenError):


Added this as an explicit way to encapsulate these types of "is default" logic errors and provide a more useful message.

NevilleS · 2024-07-31T18:36:25Z

src/fides/api/util/errors.py

+        self,
+        resource_type: str,
+        fides_key: str,
+        action: str = "modify",


This overcomplicates it a bit so that the error message says either "cannot modify", "cannot create", "cannot delete", etc.. I only did this as a bit of Python practice, to be honest.

NevilleS · 2024-07-31T18:37:25Z

src/fides/api/util/errors.py

+        error_message: str = None,
+    ) -> None:
+        error = (
+            error_message or f"cannot {action} a resource where 'is_default' is true"


The error_message or <template string> allows providing an explicit error_message argument here. I use that in one place where the error is actually more subtle- we prevent the ability to modify is_default on a resource that already exists.

NevilleS · 2024-07-31T18:37:43Z

tests/ctl/core/test_api.py

+        print(f"result object: {result}")
+        print(f"result json: {result.json()}")


Doh, should have removed these. Going to wait for the tests to finish running in CI, then will delete.

adamsachs

lgtm! clean impl of a nice, targeted improvement 👍

adamsachs · 2024-07-31T18:35:48Z

src/fides/api/util/errors.py

+        error = (
+            error_message or f"cannot {action} a resource where 'is_default' is true"
+        )
+        super().__init__(


nice OOP :)

OK good, thanks. This was a bit of Python practice 👍

cypress · 2024-07-31T19:00:27Z

Test summary

4 • 0 • 0 • 0

Run details


Project	fides
Status	Passed
Commit	`e1ce0d1`
Started	Jul 31, 2024 6:59 PM
Ended	Jul 31, 2024 7:00 PM
Duration	00:37 💡
OS	Linux Ubuntu -
Browser	Electron 106

View run in Cypress Cloud ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

NevilleS added 2 commits July 31, 2024 13:33

Improve the error messages provided when attempting to edit the is_de…

f1ff268

…fault field on taxonomy resources

Formatting, lint

274184d

NevilleS added 2 commits July 31, 2024 14:14

Update CHANGELOG

4baa369

Fix CHANGELOG

4820291

NevilleS commented Jul 31, 2024

View reviewed changes

adamsachs approved these changes Jul 31, 2024

View reviewed changes

Remove debug print statement

ddedbd8

NevilleS merged commit e1ce0d1 into main Jul 31, 2024
13 of 14 checks passed

NevilleS deleted the ns-improve-is-default-error-messages branch July 31, 2024 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve API error messages when using `is_default` field on taxonomy resources #5147

Improve API error messages when using `is_default` field on taxonomy resources #5147

NevilleS commented Jul 31, 2024 •

edited

Loading

vercel bot commented Jul 31, 2024 •

edited

Loading

cypress bot commented Jul 31, 2024 •

edited

Loading

NevilleS left a comment

NevilleS Jul 31, 2024

NevilleS Jul 31, 2024

NevilleS Jul 31, 2024

NevilleS Jul 31, 2024

NevilleS Jul 31, 2024

adamsachs left a comment

adamsachs Jul 31, 2024

NevilleS Jul 31, 2024

cypress bot commented Jul 31, 2024

		detail = {
		"error": "user does not have permission to modify",

		print(f"result object: {result}")
		print(f"result json: {result.json()}")

Improve API error messages when using is_default field on taxonomy resources #5147

Improve API error messages when using is_default field on taxonomy resources #5147

Conversation

NevilleS commented Jul 31, 2024 • edited Loading

Description Of Changes

Current Error Message

New Error Message

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Jul 31, 2024 • edited Loading

cypress bot commented Jul 31, 2024 • edited Loading

Test summary

Run details

NevilleS left a comment

Choose a reason for hiding this comment

NevilleS Jul 31, 2024

Choose a reason for hiding this comment

NevilleS Jul 31, 2024

Choose a reason for hiding this comment

NevilleS Jul 31, 2024

Choose a reason for hiding this comment

NevilleS Jul 31, 2024

Choose a reason for hiding this comment

NevilleS Jul 31, 2024

Choose a reason for hiding this comment

adamsachs left a comment

Choose a reason for hiding this comment

adamsachs Jul 31, 2024

Choose a reason for hiding this comment

NevilleS Jul 31, 2024

Choose a reason for hiding this comment

cypress bot commented Jul 31, 2024

Test summary

Run details

Improve API error messages when using `is_default` field on taxonomy resources #5147

Improve API error messages when using `is_default` field on taxonomy resources #5147

NevilleS commented Jul 31, 2024 •

edited

Loading

vercel bot commented Jul 31, 2024 •

edited

Loading

cypress bot commented Jul 31, 2024 •

edited

Loading