-
Notifications
You must be signed in to change notification settings - Fork 73
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
Handle Malformed Tokens #1523
Handle Malformed Tokens #1523
Conversation
…onfig instead of overriding fideslib.oauth.oauth_util.verify_oauth_client.
e6aee9d
to
5751fd7
Compare
Remove api/ctl/deps.py in favor of methods in ops.
I tried testing this out with the following:
During the setup of that test environment, it runs a script that attempts to authenticate an oauth client and it fails:
That used to work, which makes me think there's something not quite right with the oauth verification here. The server logs aren't very helpful:
Sorry, not sure what more to poke at to help troubleshoot... |
Thank you for testing @NevilleS I'll just work on getting unified fides setup this morning and sort it out! |
OK we didn't have a unit test for the scenario @NevilleS pointed out - which is a "root client" trying to create another client. This highlights that we can't use the We'd need to update the fideslib version to be able to pass in the scope registry to use as the root client's scopes where applicable. So after refreshing my memory https://github.com/ethyca/fidesops/pull/830/files#r916919886 it looks like no one was ever using the fideslib version of My vote is to add the proper logic to |
…t method and overriding, supply our own because we need to be able to define the root client default scopes, and there's no way to pass that into fideslib right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch and nice fix here! LGTM 😃
Test env works for me locally
Thanks for your review @ThomasLaPiana! |
Fixes #1542
Code Changes
verify_oauth_client
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
It still sounds like we're having issues with bad API tokens.
fideslib
has handling for this but its method is not usable as-is.I ended up copying over the same logic introduced in fideslib ethyca/fideslib#71 instead of using the
verify_oauth_client
method in fideslib since we can't use it without modification. It is missing some root client handling logic.My original fix was trying to import and override the correct
verify_oauth_client
function in fideslib (because there are two of them) and we were overriding the wrong one, not taking advantage of new fideslib logic.fideslib.oauth.api.deps.verify_oauth_client takes the Config and passes it to fideslib.oauth.oauth_util.verify_oauth_client.
However on closer inspection, it looks like
fideslib
doesn't have a way to receive root client scopes out of the box, so we'd have to update fideslib to be able to use itsverify_oauth_client
method. As of now, I don't think anyone has ever been using thefideslib
verify_oauth_client. I thought we were in fidesops but it looks like we never sorted that out.