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

[HOLD] Allow registering models via a setting #533

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

This is a need come up looking at @jctanner 's integration work at ansible/galaxy_ng#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.

@AlanCoding AlanCoding added the galaxy related to galaxy_ng integration label Jul 31, 2024
@john-westcott-iv john-westcott-iv added the Ready for review This PR is ready for review either initially or comments have been address label Aug 6, 2024
Copy link
Contributor

@slemrmartin slemrmartin left a comment

Choose a reason for hiding this comment

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

I like that, and the change looks pretty clear, doc is there, 👍

Fix test failure due to app readiness
Copy link

sonarcloud bot commented Aug 8, 2024

@john-westcott-iv john-westcott-iv removed the Ready for review This PR is ready for review either initially or comments have been address label Aug 20, 2024
@john-westcott-iv
Copy link
Member

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

@john-westcott-iv john-westcott-iv changed the title Allow registering models via a setting [HOLD] Allow registering models via a setting Aug 27, 2024
@AlanCoding
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
galaxy related to galaxy_ng integration Ready To Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants