-
Notifications
You must be signed in to change notification settings - Fork 81
[WIP] [AAP-54064] Decoupling apps from ansible_base.rbac #849
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
base: devel
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,5 +23,11 @@ def save(self, *args, **kwargs): | |
def service_id(): | ||
global _service_id | ||
if not _service_id: | ||
_service_id = str(ServiceID.objects.first().pk) | ||
service_obj = ServiceID.objects.first() | ||
if service_obj: | ||
_service_id = str(service_obj.pk) | ||
else: | ||
# Create a ServiceID if none exists | ||
service_obj = ServiceID.objects.create() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this is a major thing I had on my agenda. https://issues.redhat.com/browse/AAP-51352 I had assumed we couldn't do it like this. But I'm not saying no. |
||
_service_id = str(service_obj.pk) | ||
return _service_id |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
from collections import namedtuple | ||
from typing import List, Optional | ||
|
||
from django.conf import settings | ||
from django.contrib.auth import authenticate | ||
from django.utils.translation import gettext_lazy as _ | ||
|
||
|
@@ -23,12 +24,16 @@ class ServiceAPIConfig: | |
This will be the interface for configuring the resource registry for each service. | ||
""" | ||
|
||
_default_resource_processors = { | ||
"shared.team": ResourceTypeProcessor, | ||
"shared.organization": ResourceTypeProcessor, | ||
"shared.user": ResourceTypeProcessor, | ||
"shared.roledefinition": RoleDefinitionProcessor, | ||
} | ||
@classmethod | ||
def _get_default_resource_processors(cls): | ||
processors = { | ||
"shared.team": ResourceTypeProcessor, | ||
"shared.organization": ResourceTypeProcessor, | ||
"shared.user": ResourceTypeProcessor, | ||
} | ||
if 'ansible_base.rbac' in settings.INSTALLED_APPS: | ||
processors["shared.roledefinition"] = RoleDefinitionProcessor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 agreed. This should cause no ill-effects. For the major services, we simply don't care about the case that the RBAC app is not installed. |
||
return processors | ||
|
||
custom_resource_processors = {} | ||
|
||
|
@@ -43,7 +48,7 @@ def authenticate_local_user(username: str, password: str): | |
|
||
@classmethod | ||
def get_processor(cls, resource_type): | ||
combined_processors = {**cls._default_resource_processors, **cls.custom_resource_processors} | ||
combined_processors = {**cls._get_default_resource_processors(), **cls.custom_resource_processors} | ||
return combined_processors[resource_type] | ||
|
||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I somewhat disagree with this. There is no problem with making requests to the RBAC related endpoints when RBAC is not installed locally. Some methods are still invalid to call, but not all of these. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
from django.conf import settings | ||
from rest_framework import serializers | ||
from rest_framework.exceptions import ValidationError | ||
|
||
from ansible_base.rbac.models import DABContentType, DABPermission | ||
from ansible_base.resource_registry.utils.resource_type_serializers import AnsibleResourceForeignKeyField, SharedResourceTypeSerializer | ||
from ansible_base.resource_registry.utils.sso_provider import get_sso_provider_server | ||
|
||
|
@@ -84,6 +84,10 @@ class LenientPermissionSlugListField(serializers.ListField): | |
child = serializers.CharField() | ||
|
||
def to_internal_value(self, data): | ||
if 'ansible_base.rbac' not in settings.INSTALLED_APPS: | ||
raise RuntimeError("LenientPermissionSlugListField requires ansible_base.rbac to be installed") | ||
from ansible_base.rbac.models import DABPermission | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems entirely unnecessary. The rest of changes in this file, yes. |
||
|
||
data = super().to_internal_value(data) | ||
return list(DABPermission.objects.filter(api_slug__in=data)) | ||
|
||
|
@@ -98,14 +102,24 @@ class RoleDefinitionType(SharedResourceTypeSerializer): | |
name = serializers.CharField() | ||
description = serializers.CharField(default="", allow_blank=True) | ||
managed = serializers.BooleanField() | ||
content_type = serializers.SlugRelatedField( | ||
slug_field='api_slug', | ||
queryset=DABContentType.objects.all(), | ||
allow_null=True, | ||
default=None, | ||
) | ||
permissions = LenientPermissionSlugListField() | ||
|
||
def __init__(self, *args, **kwargs): | ||
if 'ansible_base.rbac' not in settings.INSTALLED_APPS: | ||
raise RuntimeError("RoleDefinitionType requires ansible_base.rbac to be installed") | ||
|
||
super().__init__(*args, **kwargs) | ||
|
||
# Set up content_type field only when rbac is available | ||
from ansible_base.rbac.models import DABContentType | ||
|
||
self.fields['content_type'] = serializers.SlugRelatedField( | ||
slug_field='api_slug', | ||
queryset=DABContentType.objects.all(), | ||
allow_null=True, | ||
default=None, | ||
) | ||
|
||
def is_valid(self, raise_exception=False): | ||
try: | ||
return super().is_valid(raise_exception=raise_exception) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,14 +18,15 @@ | |
from django.db.utils import Error, IntegrityError | ||
from requests import HTTPError | ||
|
||
from ansible_base.rbac.models.role import AssignmentBase, RoleDefinition, RoleTeamAssignment, RoleUserAssignment | ||
from ansible_base.resource_registry.models import Resource, ResourceType | ||
from ansible_base.resource_registry.models.service_identifier import service_id | ||
from ansible_base.resource_registry.registry import get_registry | ||
from ansible_base.resource_registry.rest_client import ResourceAPIClient, get_resource_server_client | ||
|
||
logger = logging.getLogger('ansible_base.resources_api.tasks.sync') | ||
|
||
_is_rbac_installed = 'ansible_base.rbac' in settings.INSTALLED_APPS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here too, I want to bring this in-line. |
||
|
||
|
||
class ManifestNotFound(HTTPError): | ||
"""Raise when server returns 404 for a manifest""" | ||
|
@@ -139,7 +140,9 @@ def fetch_manifest( | |
return [ManifestItem(**row) for row in csv_reader] | ||
|
||
|
||
def get_ansible_id_or_pk(assignment: AssignmentBase) -> str: | ||
def get_ansible_id_or_pk(assignment) -> str: | ||
if not _is_rbac_installed: | ||
raise RuntimeError("get_ansible_id_or_pk requires ansible_base.rbac to be installed") | ||
# For object-scoped assignments, try to get the object's ansible_id | ||
if assignment.content_type.model in ('organization', 'team'): | ||
object_resource = Resource.objects.filter(object_id=assignment.object_id, content_type__model=assignment.content_type.model).first() | ||
|
@@ -153,7 +156,9 @@ def get_ansible_id_or_pk(assignment: AssignmentBase) -> str: | |
return str(ansible_id_or_pk) | ||
|
||
|
||
def get_content_object(role_definition: RoleDefinition, assignment_tuple: AssignmentTuple) -> Any: | ||
def get_content_object(role_definition, assignment_tuple: AssignmentTuple) -> Any: | ||
if not _is_rbac_installed: | ||
raise RuntimeError("get_content_object requires ansible_base.rbac to be installed") | ||
content_object = None | ||
if role_definition.content_type.model in ('organization', 'team'): | ||
object_resource = Resource.objects.get(ansible_id=assignment_tuple.ansible_id_or_pk) | ||
|
@@ -167,6 +172,8 @@ def get_content_object(role_definition: RoleDefinition, assignment_tuple: Assign | |
|
||
def get_remote_assignments(api_client: ResourceAPIClient) -> set[AssignmentTuple]: | ||
"""Fetch remote assignments from the resource server and convert to tuples.""" | ||
if not _is_rbac_installed: | ||
raise RuntimeError("get_remote_assignments requires ansible_base.rbac to be installed") | ||
assignments = set() | ||
|
||
# Fetch user assignments with pagination | ||
|
@@ -238,6 +245,10 @@ def get_remote_assignments(api_client: ResourceAPIClient) -> set[AssignmentTuple | |
|
||
def get_local_assignments() -> set[AssignmentTuple]: | ||
"""Get local assignments and convert to tuples.""" | ||
if not _is_rbac_installed: | ||
raise RuntimeError("get_local_assignments requires ansible_base.rbac to be installed") | ||
from ansible_base.rbac.models.role import RoleTeamAssignment, RoleUserAssignment | ||
|
||
assignments = set() | ||
|
||
# Get user assignments | ||
|
@@ -294,6 +305,10 @@ def get_local_assignments() -> set[AssignmentTuple]: | |
|
||
def delete_local_assignment(assignment_tuple: AssignmentTuple) -> bool: | ||
"""Delete a local assignment based on the tuple.""" | ||
if not _is_rbac_installed: | ||
raise RuntimeError("delete_local_assignment requires ansible_base.rbac to be installed") | ||
from ansible_base.rbac.models.role import RoleDefinition | ||
|
||
try: | ||
role_definition = RoleDefinition.objects.get(name=assignment_tuple.role_definition_name) | ||
|
||
|
@@ -320,6 +335,10 @@ def delete_local_assignment(assignment_tuple: AssignmentTuple) -> bool: | |
|
||
def create_local_assignment(assignment_tuple: AssignmentTuple) -> bool: | ||
"""Create a local assignment based on the tuple.""" | ||
if not _is_rbac_installed: | ||
raise RuntimeError("create_local_assignment requires ansible_base.rbac to be installed") | ||
from ansible_base.rbac.models.role import RoleDefinition | ||
|
||
try: | ||
role_definition = RoleDefinition.objects.get(name=assignment_tuple.role_definition_name) | ||
|
||
|
@@ -694,6 +713,10 @@ def _sync_assignments(self): | |
if not self.sync_assignments: | ||
return | ||
|
||
if not _is_rbac_installed: | ||
self.write(">>> Skipping role assignments sync (rbac not installed)") | ||
return | ||
|
||
self.write(">>> Syncing role assignments") | ||
|
||
try: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really want to stop doing this referencing of
settings.
on import. It's an import circularity problem, and here you don't need it anyway. You can make this into a method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea we do this all over. We might want to just have a generic method like
is_dab_app_installed('rbac')