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

RBAC system with custom roles integrated with AWX #45

Merged
merged 14 commits into from
Mar 18, 2024

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented Dec 8, 2023

This rewrites the roles system inside of AWX and moves them here, to this library, so that ideally we can connect it to other apps. I want to be very clear that this is mainly an object-role system. I added functionality to handle system-wide permissions, but this is a different sub-system.

AWX patch to use this - ansible/awx#14735

The only reason this is possible at all is because most of the AWX gnarly permission logic exists on a layer of business logic fairly well-separated from the internals of the RBAC system. It's mainly the internals that this offers (although it tacked on a simple API), for object-roles - roles to things. Those things can be in a shallow hierarchy... which is just organizations right now. Inheritance of permissions from more than 1 parent resource has some basic tests, but isn't expected to be used yet.

Right now this is fully functional in AWX, with only 2-3 known integration test failures due to sacrifices that were knowingly made.

TODO:

  • Add views for this new system. Current blocker is that basic details of the API contract are not be agreed on yet. The AWX branch offers a compatibility layer with the old roles system.
  • Add content_type to RoleDefinition. This will be used for validation of permissions, because listing some permissions require listing other permissions, and most permissions are only valid for a certain model (or a parent of that model).
  • Fix unit tests in AWX. The compatibility layer currently doesn't do this for us.
  • Reconcile the team & organization members relationship that already exists in Gateway compatible with this. The role system, today, handles this by putting users in a pre-created team member role. If the role is the source of truth, then we need to add some ORM shortcuts to more easily access it.
  • Very large item - migrate galaxy_ng to this system like the AWX migration. This would have to go along with the introduction of an organization model, without organizations this entire system is useless. Ping @cutwater
  • Add to the AWX migration - split the organization admin role into 2 with 1 dealing with orgs / teams / user management, and the other dealing with AWX objects alone. This role will then be migrated into Gateway itself (not ansible_base). Oh yeah, and also convert Gateway to this system... I don't think it has a permissions system right now. Not doing this
  • Can we give this a name? DAB RBAC

Copy link
Member

@newswangerd newswangerd left a comment

Choose a reason for hiding this comment

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

This looks great. I'd like to merge the resource registry and then update this to integrate with the resource registry system. I think once we do that, I'm happy to merge this PR.

ansible_base/models/rbac.py Outdated Show resolved Hide resolved
ansible_base/models/rbac.py Outdated Show resolved Hide resolved
docs/rbac.md Outdated Show resolved Hide resolved
ansible_base/rbac/caching.py Outdated Show resolved Hide resolved
ansible_base/rbac/caching.py Outdated Show resolved Hide resolved
ansible_base/rbac/caching.py Outdated Show resolved Hide resolved
@AlanCoding AlanCoding force-pushed the django_permissions branch 4 times, most recently from 3140eb5 to 8d36bee Compare January 26, 2024 03:46
@AlanCoding AlanCoding changed the title [PoC] RBAC system with custom roles integrated with AWX RBAC system with custom roles integrated with AWX Jan 26, 2024
@AlanCoding AlanCoding force-pushed the django_permissions branch 4 times, most recently from 5cf23f3 to 90381bb Compare January 31, 2024 15:24
@john-westcott-iv john-westcott-iv changed the title RBAC system with custom roles integrated with AWX [WIP] RBAC system with custom roles integrated with AWX Feb 1, 2024
Copy link
Member

@john-westcott-iv john-westcott-iv left a comment

Choose a reason for hiding this comment

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

I've not combed over every file but wanted to get a start ont he comments. One general question is the layout of the files inside rbac. FIrst there is an api directory but there are some things not in there like caching and evaluations. The other thing is we have many classes in single views/serializers/modles files. I much prefer a directory with seperate files if possible. But those are kind of personal preferences.

If you want to stick with this layout for this PR I am ok with that but maybe we want to have a "standard" layout for apps in DAB in general?

ansible_base/lib/utils/models.py Show resolved Hide resolved
ansible_base/lib/utils/models.py Show resolved Hide resolved
ansible_base/lib/utils/models.py Show resolved Hide resolved
docs/apps/rbac.md Show resolved Hide resolved
docs/apps/rbac.md Outdated Show resolved Hide resolved
docs/apps/rbac.md Show resolved Hide resolved
docs/apps/rbac.md Show resolved Hide resolved
docs/apps/rbac.md Show resolved Hide resolved
ansible_base/rbac/models.py Show resolved Hide resolved
ansible_base/rbac/models.py Show resolved Hide resolved
@newswangerd
Copy link
Member

I'm going to go ahead and approve this, though I do think we need to have more conversations around the permissions model.

@AlanCoding
Copy link
Member Author

Does it make more sense to use a foreign key now that we have a concrete permission model?

I wanted to do that before today, but workload from other rebasing and other fixes kept me from it. Described in #222

@AlanCoding
Copy link
Member Author

@john-westcott-iv AlanCoding#4 is ready, in that it is passing tests

If you want to stick with this layout for this PR I am ok with that but maybe we want to have a "standard" layout for apps in DAB in general?

I think it's a pretty clear practice that models.py is common, but with multiple categories, beyond a certain scale you can consider a models/ folder. That PR does that. I'm not super convinced that it is better, but I'm okay with it. I can push it here before merge if people want that.

Key features are
 - support of resource tree permission inheritance
 - support for grouping users into teams
 - efficient generation of querysets
 - endpoints for RBAC models, permission classes

Added feature for tracking roles with relationships

Added feature to manage singleton permissions

Add util for assigning permissions in migrations
  add util for creating permissions in custom model

Use null content_type to identify system roles

From review
Use shared method for add permission determination
Make the RBAC_checks command more pretty
  give non-zero exit code if issues are identified
Use " instead of ' for block strings
Add model to test parent names not matching model
Fix bug with stale org-obj evaluations from review
Be more consistent with ContentType usage
Fix test failure from rebase due to postgres and flake8
Avoid database .get for unsaved objects in pre_save
  discovered via eda-server testing
Fix RBAC_checks management command
  when the thing it is doing is disabled by settings

addition:
Add another setting for disabling custom system role creation
Add plus integration docs for this new feature

Add tests for using unauthenticated client with some views
Assure that user viewset has minimal functionailty with generic permission class
* Initial add of permission model

* Add an early exit for permission creation method
Adopt new created and modified fields in CommonModel
  after rebase - supporting changes
Copy link

@AlanCoding AlanCoding merged commit d45d5a1 into ansible:devel Mar 18, 2024
7 checks passed
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.

7 participants