ansible / django-ansible-base

Apache License 2.0
12 stars 43 forks source link

[HOLD] Allow registering models via a setting #533

Open AlanCoding opened 1 month ago

AlanCoding commented 1 month ago

This is a need come up looking at @jctanner 's integration work at https://github.com/ansible/galaxy_ng/pull/2202

See the management command in particular and stuff like:

        galaxy_role_description = {
            'core.task_owner': 'Allow all actions on a task.',
            'core.taskschedule_owner': 'Allow all actions on a task schedule.',
            'galaxy.ansible_repository_owner': 'Manage ansible repositories.',
            'galaxy.collection_admin': 'Create, delete and change collection namespaces. Upload and delete collections. Sync collections from remotes. Approve and reject collections.',
            'galaxy.collection_curator': 'Approve, reject and sync collections from remotes.',
            'galaxy.collection_namespace_owner': 'Change and upload collections to namespaces.',
            'galaxy.collection_publisher': 'Upload and modify collections.',
            'galaxy.collection_remote_owner': 'Manage collection remotes.',
            'galaxy.content_admin': 'Manage all content types.',
            'galaxy.execution_environment_admin': 'Push, delete and change execution environments. Create, delete and change remote registries.',
            'galaxy.execution_environment_collaborator': 'Change existing execution environments.',
            'galaxy.execution_environment_namespace_owner': 'Create and update execution environments under existing container namespaces.',
            'galaxy.execution_environment_publisher': 'Push and change execution environments.',
            'galaxy.group_admin': 'View, add, remove and change groups.',
            'galaxy.synclist_owner': 'View, add, remove and change synclists.',
            'galaxy.task_admin': 'View and cancel any task.',
            'galaxy.user_admin': 'View, add, remove and change users.',
        }

This seems to get us a list of "app_label.model_name" references of models. The current advice is to call permission_registry.register passing the model classes for all of those. Without getting into the technical constraints, there seems to be a pretty clear preference to just list these in a setting, which DAB RBAC can obvious handle without import-order-changing stuff.

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

john-westcott-iv commented 3 weeks ago

This may no longer be necessary. Waiting to see if its needed or not.

AlanCoding commented 1 week ago

It's not necessarily, but I might still like to do this, and take the entire project in this direction.

But we should either agree we like this, and double-down further on it, or quit.

How do we double-down? Because right now permission_registry._registry is a set of model classes. But Django doesn't really like to do this. Instead, a more robust implementation would be that we only register the (app_label, model_name) of the models. Then whenever permission_registry does anything, it uses Django apps to look up the model.

Why the heck would we do this? Because that would allow using apps dynamically, meaning that we could use the permission registry with apps inside of a migration. That means we could work with a historical state.

That raises even more questions, because models will be modified and removed over time, as the state of the app changes... so does that mean you would create permission_registry objects inside of migrations? Umm... yes, you would. But you could only do this if you saved the prior registrations. But again, yes, you should.

Big picture, then, DAB RBAC may do well to have a formalized way to record the historical state of registrations.