Fideslib models #700
Fideslib models #700
Conversation
|
Note that I added #792 as a follow-up ticket from this work. I'd like to get this important refactor merged in, even if it means we should temporarily disable the |
There was a problem hiding this comment.
Great work on this @sanders41 . I especially appreciate the tidy-up work you've done along the way, like adding types, and removing unneeded [] using python's all().
I have a couple minor questions on approach, but the major thing I'd like answered is- Now that we have exceptions from fideslib AND fidesops, when should we use fideslib exceptions vs fidesops exceptions?
src/fidesops/schemas/jwt.py
Outdated
| JWE_PAYLOAD_CLIENT_ID = "client-id" | ||
| JWE_PAYLOAD_SCOPES = "scopes" | ||
| JWE_ISSUED_AT = "iat" | ||
| # JWE_PAYLOAD_CLIENT_ID = "client-id" |
There was a problem hiding this comment.
can we remove this file?
There was a problem hiding this comment.
Looks like I missed this one. I'll remove it.
| total_users = 25 | ||
| for i in range(total_users): | ||
| body = { | ||
| "username": f"user{i}@example.com", | ||
| "password": str_to_b64_str("Password123!"), |
There was a problem hiding this comment.
str_to_b64_str was added recently by @TheAndrewJackson 's password hashing update. It would be great to keep these to properly replicate functionality in our testing. What do you think?
There was a problem hiding this comment.
I was getting test failures when I had str_to_b64_str in the test. I'll see if I can figure out why.
There was a problem hiding this comment.
Innnteresting. I'd prefer to follow-up with this, so we can merge this in sooner rather than later.
There was a problem hiding this comment.
We need to add some of this str_to_b64_str logic to fideslib now, and then revert these tests back to expecting the password to be base64 encoded when they're passed in ethyca/fideslib#43
There was a problem hiding this comment.
Ah I created two tickets already for this, one for the fideslib side, one for the fidesops side
There was a problem hiding this comment.
this makes sense, since #809 is a regression. Thanks!
|
For the exceptions question, the exceptions were already created when I started working on fideslib and the ones that were there looked like the ones that would apply to multiple libraries. If there are any exceptions left in fidesops that you think are generic and we should also move to fideslib we can do that. |
|
OK looks like we just need to push out an update to delete `src/fidesops/schemas/jwt.py. I've created #803 to account for cleanup work. |
* WIP * WIP * WIP * WIP * Use available exceptions from fideslib * Fix failing tests * Fix policy tests * Remove debugging code * Fix failing tests * Fix failing tests * Fix failiing tests * Run black and isort * Make pylint in docker happy * Clean up migrations * Move downgrade point of table renames * Remove Dockerfile temp workaround and fix pylint errors * Fix failing tests * Remove jwt.py Co-authored-by: Paul Sanders <[email protected]>
❗ Contains a migration
Purpose
Replace user related models and schemas from fidesops with those from fideslib.
Changes
Checklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #460