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

Translate new RBAC to old RBAC #15490

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

fosterseth
Copy link
Member

@fosterseth fosterseth commented Sep 4, 2024

SUMMARY

User and Team assignments using the DAB RBAC system will be translated back to the old Role system.

This ensures better backward compatibility and addresses some inconsistences in the UI that were relying on older RBAC endpoints.

I don't have a lot of tests around this yet.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

'WorkflowJobTemplate Approve': 'approval_role',
'InstanceGroup Admin': 'admin_role',
'InstanceGroup Use': 'use_role',
# 'ExecutionEnvironment Admin': '',
Copy link
Member Author

Choose a reason for hiding this comment

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

no roles on EE - not sure how to handle this one

Copy link
Member

Choose a reason for hiding this comment

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

These are new, assignments to those should not change anything in the old system.

'Inventory Use': 'use_role',
'Inventory Adhoc': 'adhoc_role',
'Inventory Update': 'update_role',
# 'NotificationTemplate Admin': 'admin_role',
Copy link
Member Author

Choose a reason for hiding this comment

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

same as EE

if delete:
instance.team.member_role.children.remove(role)
else:
instance.team.member_role.children.add(role)
Copy link
Member Author

Choose a reason for hiding this comment

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

team member_role vs admin_role?

Copy link
Member

Choose a reason for hiding this comment

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

just member_role, not admin_role. The admin role is a parent of member role, so any parent-child relationships to other roles (like, you gave the team a permission) only needed to happen for the member role. However, what I sought to establish in #15462 is that there are extremely so minimal observable API behaviors related to the old parents/children relationship, that it can already be removed. What you have is fine.

User and Team assignments using the DAB
RBAC system will be translated back to the old
Role system.

This ensures better backward compatibility and
addresses some inconsistences in the UI that were
relying on older RBAC endpoints.

Signed-off-by: Seth Foster <fosterbseth@gmail.com>
Signed-off-by: Seth Foster <fosterbseth@gmail.com>
@fosterseth fosterseth force-pushed the translate_new_to_old_rbac branch from 415bc41 to f71dca7 Compare September 4, 2024 19:12
with disable_rbac_sync():
field_name = ROLE_DEFINITION_TO_ROLE_FIELD.get(instance.role_definition.name)
if not field_name:
return
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a debug-level log here might be good to have around. This is a perfectly normal thing to happen, but for new upgrades this is probably the exception, not the norm. So it would be good to track.


url = get_relative_url('roleuserassignment-detail', kwargs={'pk': user_assignment.id})
delete(url, user=admin, expect=204)
assert bob not in team.member_role.members.all()
Copy link
Member

Choose a reason for hiding this comment

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

Testing here is a little light. We should cover more of the code branches in the signals added.

@AlanCoding
Copy link
Member

Here are my suggested tests for this:

from ansible_base.rbac.models import RoleDefinition, RoleUserAssignment, RoleTeamAssignment
from ansible_base.lib.utils.response import get_relative_url


@pytest.mark.django_db
class TestNewToOld:
    def test_new_to_old_rbac_addition(self, admin, post, inventory, bob, setup_managed_roles):
        rd = RoleDefinition.objects.get(name='Inventory Admin')

        url = get_relative_url('roleuserassignment-list')
        post(url, user=admin, data={
            'role_definition': rd.id,
            'user': bob.id,
            'object_id': inventory.id
        }, expect=201)
        assert bob in inventory.admin_role.members.all()

    def test_new_to_old_rbac_removal(self, admin, delete, inventory, bob, setup_managed_roles):
        inventory.admin_role.members.add(bob)

        rd = RoleDefinition.objects.get(name='Inventory Admin')
        user_assignment = RoleUserAssignment.objects.get(user=bob, role_definition=rd, object_id=inventory.id)

        url = get_relative_url('roleuserassignment-detail', kwargs={'pk': user_assignment.id})
        delete(url, user=admin, expect=204)
        assert bob not in inventory.admin_role.members.all()

    def test_new_to_old_rbac_team_member_addition(self, admin, post, team, bob, setup_managed_roles):
        rd = RoleDefinition.objects.get(name='Controller Team Member')

        url = get_relative_url('roleuserassignment-list')
        post(url, user=admin, data={
            'role_definition': rd.id,
            'user': bob.id,
            'object_id': team.id
        }, expect=201)
        assert bob in team.member_role.members.all()

    def test_new_to_old_rbac_team_member_removal(self, admin, delete, team, bob):
        'Team membership removals in new RBAC should be reflected in old RBAC'
        team.member_role.members.add(bob)

        rd = RoleDefinition.objects.get(name='Controller Team Member')
        user_assignment = RoleUserAssignment.objects.get(user=bob, role_definition=rd, object_id=team.id)

        url = get_relative_url('roleuserassignment-detail', kwargs={'pk': user_assignment.id})
        delete(url, user=admin, expect=204)
        assert bob not in team.member_role.members.all()

    def test_new_to_old_rbac_team_addition(self, admin, post, team, inventory, setup_managed_roles):
        rd = RoleDefinition.objects.get(name='Inventory Admin')

        url = get_relative_url('roleteamassignment-list')
        post(url, user=admin, data={
            'role_definition': rd.id,
            'team': team.id,
            'object_id': inventory.id
        }, expect=201)
        assert team.member_role in inventory.admin_role.parents.all()

    def test_new_to_old_rbac_team_removal(self, admin, delete, team, inventory, setup_managed_roles):
        inventory.admin_role.parents.add(team.member_role)

        rd = RoleDefinition.objects.get(name='Inventory Admin')
        team_assignment = RoleTeamAssignment.objects.get(team=team, role_definition=rd, object_id=inventory.id)

        url = get_relative_url('roleteamassignment-detail', kwargs={'pk': team_assignment.id})
        delete(url, user=admin, expect=204)
        assert team.member_role not in inventory.admin_role.parents.all()

I really like the congruence of the 6 test cases. You could consolidate them by parameterization if you want, either way. On devel branch these all fail with the queryset assertion, as we would want:

FAILED awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py::TestNewToOld::test_new_to_old_rbac_addition - assert <User: bob> in <QuerySet []>
FAILED awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py::TestNewToOld::test_new_to_old_rbac_removal - assert <User: bob> not in <QuerySet [<User: bob>]>
FAILED awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py::TestNewToOld::test_new_to_old_rbac_team_member_addition - assert <User: bob> in <QuerySet []>
FAILED awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py::TestNewToOld::test_new_to_old_rbac_team_member_removal - assert <User: bob> not in <QuerySet [<User: bob>]>
FAILED awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py::TestNewToOld::test_new_to_old_rbac_team_addition - assert <Role: Member-17> in <QuerySet [<Role: Inventory Admin-4>]>
FAILED awx/main/tests/functional/dab_rbac/test_dab_rbac_api.py::TestNewToOld::test_new_to_old_rbac_team_removal - assert <Role: Member-17> not in <QuerySet [<Role: Inventory Admin-4>, <Role: Member-17>]>

I have not tested with your branch, but I expect it to change all of these to passing.

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.

Left a comment with a bunch of tests. I'd like to see those passing, and if they do then this all looks good to go.

@fosterseth fosterseth requested a review from elyezer September 5, 2024 12:59
Copy link
Member

@elyezer elyezer left a comment

Choose a reason for hiding this comment

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

This seems only focused on the application code, so from the build breakage perspective it looks good.

Also this is targeting the devel branch which we are not using for the nightly runs.

@AlanCoding
Copy link
Member

The collection integration test failure has:

The host sent back a server error (HTTP Error 500: Internal Server Error): /api/v2/users/2/. Please check the logs and try again later

This suggests that we should dive a bit deeper into server logs, and that fishing for tracebacks may be productive.

@AlanCoding
Copy link
Member

https://gist.github.com/AlanCoding/8579e072400e07d6347e43611600d319

From the checks, this is the server error. Indeed, it looks unrelated to this change. But it could be related.

@AlanCoding
Copy link
Member

The tasks I believe are relevant, or potentially relevant, to the error:

- name: Create a User
user:
first_name: Joe
last_name: User
username: "{{ username }}"
password: "{{ 65535 | random | to_uuid }}"
email: joe@example.org
state: present
register: result

- name: Add Joe as execution admin to Default Org.
role:
user: "{{ username }}"
users:
- "{{ username }}2"
role: execution_environment_admin
organizations: Default
state: "{{ item }}"
register: result
with_items:
- "present"
- "absent"

- name: Add Joe to workflow execute role
role:
user: "{{ username }}"
users:
- "{{ username }}2"
role: execute
workflow: test-role-workflow
job_templates:
- jt1
- jt2
state: present
register: result

- name: Delete a User
user:
username: "{{ username }}"
email: joe@example.org
state: absent
register: result

Signed-off-by: Seth Foster <fosterbseth@gmail.com>
@fosterseth fosterseth force-pushed the translate_new_to_old_rbac branch from a99f163 to 5488680 Compare September 5, 2024 17:31
Signed-off-by: Seth Foster <fosterbseth@gmail.com>
Signed-off-by: Seth Foster <fosterbseth@gmail.com>
Copy link

sonarqubecloud bot commented Sep 5, 2024

@fosterseth fosterseth merged commit c4d8fdb into ansible:devel Sep 6, 2024
19 checks passed
@fosterseth
Copy link
Member Author

YOLO passing

djyasin pushed a commit to djyasin/awx that referenced this pull request Nov 11, 2024
User and Team assignments using the DAB
RBAC system will be translated back to the old
Role system.

This ensures better backward compatibility and
addresses some inconsistences in the UI that were
relying on older RBAC endpoints.

Signed-off-by: Seth Foster <fosterbseth@gmail.com>
Co-authored-by: Alan Rominger <arominge@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants