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

DIG-1178: New AuthX model #9

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from
Draft

DIG-1178: New AuthX model #9

wants to merge 13 commits into from

Conversation

justin-ys
Copy link
Contributor

@justin-ys justin-ys commented May 31, 2023

  • Adds is_permissible(request, dataset) method to validate individual requests
  • Changes get_opa_datasets to get_readable_datasets
  • Deprecates is_site_admin (still used internally until OPA update is ready)
  • Backwards compatible, but warnings are provided for deprecated behavior
  • Changes/appends tests appropriately

@justin-ys justin-ys changed the title New AuthX model DIG-1178: New AuthX model Jun 1, 2023
@kcranston
Copy link
Member

There was some discussion about refactoring the is_permissible method to accept either a python request object, or a json object containing the data. This would make it a non-breaking change for other services.

@justin-ys
Copy link
Contributor Author

I think is_permissible won't accept a request object, since it's a new function anyway so nothing would be using it (except my branches, which have already moved to the new request model anyway). But this has been implemented for all other functions

@justin-ys justin-ys marked this pull request as ready for review July 21, 2023 18:39
@justin-ys justin-ys requested a review from daisieh July 21, 2023 18:39
@kcranston
Copy link
Member

kcranston commented Jul 21, 2023

I am working on the OPA side now. We can't merge this until that's done, because the is_permissible stub is, well, too permissible and would make integration tests fail. Specifically, I don't want to have this merged into develop:

   elif request["method"] == 'GET':
        return True

Open to suggestions for a safer workaround that would let the rest of the data ingest project proceed.

@justin-ys justin-ys marked this pull request as draft July 25, 2023 13:23
@kcranston kcranston changed the base branch from main to develop August 28, 2023 17:23
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