Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing root user client ID #4582

Merged
merged 10 commits into from
Feb 2, 2024
Merged

Fixing root user client ID #4582

merged 10 commits into from
Feb 2, 2024

Conversation

galvana
Copy link
Contributor

@galvana galvana commented Jan 31, 2024

Closes PROD-1540 and PROD-1541

Description Of Changes

Removing the need for a client_id when creating of a Policy, Rule, or RuleTarget. The root user is transient and doesn't have an entry in the client database table. This means that the client_id associated with the root user's ClientDetail does not refer to a database row and causes the policy and related tables to raise a foreign key violation during creation. Met with @NevilleS and @adamsachs and agreed it was acceptable to remove the client_id from the create calls for now. We want to spend more time at a later date to determine the full impact of removing the client_id from the tables altogether.

Code Changes

  • Removing the need for a client_id when creating of a Policy, Rule, or RuleTarget
  • Updating the client_id field to be optional on the Policy model
  • Added tests that create Policy, Rule, or RuleTarget as a root user

Steps to Confirm

  • Start fides nox -s dev
  • Navigate to http://localhost:8080/docs
  • Authenticate using the fidesadmin user
  • Attempt to create a new Policy, Rule, or RuleTarget, verify there aren't any errors

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

Copy link

vercel bot commented Jan 31, 2024

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

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 2, 2024 5:50pm

Copy link

cypress bot commented Jan 31, 2024

Passing run #6157 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 5e38912 into 67a515a...
Project: fides Commit: 2168f3263e ℹ️
Status: Passed Duration: 00:34 💡
Started: Feb 1, 2024 6:32 PM Ended: Feb 1, 2024 6:33 PM

Review all test suite changes for PR #4582 ↗︎

@galvana galvana changed the title First pass Fixing root user client ID Jan 31, 2024
@galvana galvana marked this pull request as ready for review February 1, 2024 18:45
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @galvana. This is a very old artifact!

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (67a515a) 86.81% compared to head (5e38912) 86.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4582   +/-   ##
=======================================
  Coverage   86.81%   86.81%           
=======================================
  Files         330      330           
  Lines       19839    19839           
  Branches     2545     2545           
=======================================
  Hits        17223    17223           
  Misses       2150     2150           
  Partials      466      466           

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

@galvana galvana merged commit 3bacb77 into main Feb 2, 2024
6 of 7 checks passed
@galvana galvana deleted the PROD-1541-fix-root-user-client-id branch February 2, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants