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

add backend ability to scope API keys to a particular Role #28420

Merged
merged 17 commits into from
Aug 26, 2020
Merged

Conversation

czue
Copy link
Member

@czue czue commented Aug 25, 2020

#28388

SUMMARY

This adds backend support for attaching a role_id to an API key, and then limiting the access of that key to the associated roles.

I ended up going with Roles as the mapping layer because it was the easiest way to bundle a collection of permissions, and also because it will be the easiest thing to support in the (yet to be added) UI. Any other model would have required either very granular scopes (at the permission level) or creating PermissionCollection abstraction that is basically the exact same thing as a Role. This way we can leverage all the Role editing stuff we have and just add a dropdown in the API key UI.

You can review either way (might be easier all at once). The change itself is rather small and the meat of it is in 854638d. I'm open to other ways of rolling it out that are backwards compatible and safe. Most of the rest of the PR is tests and fixes related to that change.

As advertised on the CEP, this only affects tasty pie APIs and those that use either RequirePermissionAuthentication or LoginAndDomainAuthentication, though am hoping to extend support in other places.

RISK ASSESSMENT / QA PLAN

It's core authentication code so there is always a risk there. I added substantial test coverage and manually tested various APIs with various types of auth locally. Due to they way it is implemmented, this should not affect any non-API-based auth in any way.

There is also a small DB migration to add role_id to the API key model

PRODUCT DESCRIPTION

Not user-facing yet, but hopefully will be soon.

@czue czue added the product/all-users-all-environments Change impacts all users on all environments label Aug 25, 2020
@czue czue added the reindex/migration Reindex or migration will be required during or before deploy label Aug 25, 2020
Copy link
Member

@ctsims ctsims left a comment

Choose a reason for hiding this comment

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

This approach seems consistent to me.

Just as a check for my understanding, the assumption is that for "mixed auth" endpoints like

Any required permissions check would use the universal "require_api_permission" check which will fall back to the normal has_permissions logic if there's no API key, rather than bifurcating, right? Am wondering whether it's possible to chain the @api_access decorator in some way that lets future permissions requests know to apply the api permissions layer 2 check without forcing both a

@require_can_edit_data
and an
@require_can_edit_data_api

wrapper around require_permission and require_api_permission respectively,

@czue czue merged commit 3e13a33 into master Aug 26, 2020
@czue czue deleted the cz/api-scopes branch August 26, 2020 07:01
@czue
Copy link
Member Author

czue commented Aug 26, 2020

@ctsims I'm hoping the eventual future state will be that you add @require_can_edit_data to a view, and then the logic goes something like:

if authed_with_api_key():
  ensure_api_key_has_right_permission()
else:
  ensure_user_has_right_permission()

Is that the same as you're suggesting?

(This is already how it works with the APIs in this PR, but I didn't yet touch the @api_auth decorator.)

@ctsims
Copy link
Member

ctsims commented Aug 26, 2020

@czue Yeah, that's the same as what I'm suggesting, just making sure that my reading was correct that this approach wouldn't extend the need to differentiate between the permissions granted by the allowable auth types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/all-users-all-environments Change impacts all users on all environments reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants