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

Make controller specific team and org roles #15445

Merged
merged 15 commits into from
Aug 22, 2024

Conversation

fosterseth
Copy link
Member

@fosterseth fosterseth commented Aug 14, 2024

SUMMARY

Controller Team Admin
Controller Team Member
Controller Organization Admin
Controller Organization Member

Permissions-wise, these four roles are identical to the platform roles, those without the Controller prefix.

Role assignments that are issued via the AWX API utilize these role definitions, not the platform roles. Role assignments for the platform roles must be issued in gateway and synced to AWX via JWT payloads. A followup PR will add a flag and some logic to role definitions to implement this restriction.

For role assigments that use the deprecated RBAC system, the Controller specific roles will be used.

ISSUE TYPE
  • New or Enhanced Feature
COMPONENT NAME
  • API

TODO

  • make sure migrate_to_new_rbac is using these roles

@AlanCoding
Copy link
Member

I'd like to get some API-level tests (like with the get, post fixtures), and repeating them here:

  • POST to /api/v2/roles/:id/members/ where :id is the member role for a team should "work", where the user id is in the request data
  • POST to /api/v2/role_user_assignments/ giving "Team Member" role to a user should fail with a 400
  • POST to /api/v2/role_user_assignments/ giving "Controller Team Member" role to a user, to a team, should work

Tests in ansible/django-ansible-base#562 overlap with this, but don't really do it justice with the specific names and stuff. Your existing test does what the 1st case does on the ORM layer. But I'd like to confirm the API version of all of these cases work using the test database.

@AlanCoding
Copy link
Member

The test failures from this are strange, basically all permission checks are giving the right answer. I don't have a simple answer as to why that's happening.

@AlanCoding
Copy link
Member

I put up #15453 and I believe this will make some progress towards fixing the test failures. Probably not all, but it goes in the right direction, in that we remove any references to (old) role members and use permission evaluations instead. We needed to do this anyway, irrelevant of this.

AlanCoding added a commit to ansible/django-ansible-base that referenced this pull request Aug 21, 2024
I was more worried about the revoking flow, than the granting flow.

But it turned out it worked just fine. So I'll go ahead and add these,
in support of ansible/awx#15445

AAP-24052
@fosterseth fosterseth force-pushed the add_controller_specific_roles branch from 8dd8d51 to e97175e Compare August 21, 2024 19:38
@@ -123,6 +126,10 @@ def get_absolute_url(self, request=None):
def _get_related_jobs(self):
return UnifiedJob.objects.non_polymorphic().filter(organization=self)

def validate_role_assignment(self, actor, role_definition):
if role_definition.name in ['Organization Admin', 'Organization Member']:
raise DRFValidationError({'detail': _(f"Assignment must use the Controller {role_definition.name} role.")})
Copy link
Member

Choose a reason for hiding this comment

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

Remove this validation. It'll be handled in DAB and has to be configurable.

Copy link
Member

Choose a reason for hiding this comment

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

With the new DAB version, you should be able to add the setting that will replace this:

ALLOW_LOCAL_ASSIGNING_JWT_ROLES = False

@@ -166,6 +173,10 @@ class Meta:
def get_absolute_url(self, request=None):
return reverse('api:team_detail', kwargs={'pk': self.pk}, request=request)

def validate_role_assignment(self, actor, role_definition):
if role_definition.name in ['Team Admin', 'Team Member']:
raise DRFValidationError({'detail': _(f"Assignment must use the Controller {role_definition.name} role.")})
Copy link
Member

Choose a reason for hiding this comment

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

Also remove this.


new_state = migrator.apply_tested_migration(
('main', '0192_custom_roles'),
)

RoleUserAssignment = new_state.apps.get_model('dab_rbac', 'RoleUserAssignment')
assert RoleUserAssignment.objects.filter(user=user.id, object_id=org.id).exists()
assert RoleUserAssignment.objects.filter(user=user.id, role_definition__name='Controller Organization Member', object_id=org.id).exists()
assert RoleUserAssignment.objects.filter(user=user.id, role_definition__name='Controller Team Member', object_id=team.id).exists()
Copy link
Member

Choose a reason for hiding this comment

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

👍 alright, looks good here.

Copy link
Member

Choose a reason for hiding this comment

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

Did that not require any changes to the data migration? I guess maybe it imported some of the translation methods from the models? I just don't see how this would have gotten fixed.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Aug 22, 2024
Controller Team Admin
Controller Team Member
Controller Organization Admin
Controller Organization Member

Plug old RBAC system to use these roles
instead of the platform roles.

Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
Signed-off-by: Seth Foster <[email protected]>
@fosterseth fosterseth force-pushed the add_controller_specific_roles branch from 2bb44a9 to bcef496 Compare August 22, 2024 18:56
Signed-off-by: Seth Foster <[email protected]>
Copy link

sonarcloud bot commented Aug 22, 2024

Copy link
Member

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

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

Covered migrations

We have the validation we need in place for the new (DAB RBAC) API.

And the core work, of connecting translation layer to the new roles, seems to be sufficiently covered by the tests.

AFAIK, this looks code-complete to me.

@fosterseth fosterseth merged commit 7ed0eee into ansible:devel Aug 22, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants