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

system auditor role #2162

Closed
wants to merge 9 commits into from
Closed

Conversation

jctanner
Copy link
Collaborator

@jctanner jctanner commented Jun 10, 2024

https://issues.redhat.com/browse/AAP-24730

replaces #2157

[root@35f0b6ad6e2c src]# pulpcore-manager shell < script.py 

Collection Namespaces
        ADD :: 467 :: Can view namespace :: view_namespace

Collections
        ADD :: 17 :: Can view collection :: view_collection
        ADD :: 29 :: Can view ansible repository :: view_ansiblerepository

Users
        no changes needed

Groups
        no changes needed

Collection Remotes
        no changes needed

Ansible Repository
        no changes needed

Execution Environments
        ADD :: 161 :: View any push repository in a namespace :: namespace_view_containerpushrepository
        ADD :: 135 :: Can view container repository :: view_containerrepository
        ADD :: 154 :: Can view container namespace :: view_containernamespace
        ADD :: 508 :: Can view container namespace :: view_containernamespace
        ADD :: 157 :: View any distribution in a namespace :: namespace_view_containerdistribution

Container Registry Remotes
        ADD :: 500 :: Can view container registry remote :: view_containerregistryremote

Task Management
        no changes needed

image

No-Issue

Signed-off-by: James Tanner <[email protected]>
@github-actions github-actions bot added backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) backport-4.9 This PR should be backported to stable-4.9 (2.4) labels Jun 10, 2024
No-Issue

Signed-off-by: James Tanner <[email protected]>
jctanner added 4 commits June 10, 2024 17:36
No-Issue

Signed-off-by: James Tanner <[email protected]>
No-Issue

Signed-off-by: James Tanner <[email protected]>
No-Issue

Signed-off-by: James Tanner <[email protected]>
No-Issue

Signed-off-by: James Tanner <[email protected]>
@jctanner jctanner removed backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) backport-4.9 This PR should be backported to stable-4.9 (2.4) multi-commit labels Jun 10, 2024
No-Issue

Signed-off-by: James Tanner <[email protected]>
No-Issue

Signed-off-by: James Tanner <[email protected]>
@@ -30,6 +30,12 @@ class DeploymentMode(enum.Enum):
# Category to group the permission in the UI.
"ui_category": _("Collection Namespaces"),
},
"galaxy.view_namespace": {
Copy link
Member

Choose a reason for hiding this comment

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

If you add a new permission here, would you need to add it to existing roles like galaxy.collection_namespace_owner which currently offers galaxy.change_namespace? Would some other API access logic need to be rewritten to utilize the new permission?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AlanCoding I was pointed to this file just last night and I don't fully know how it works.

Copy link
Member

Choose a reason for hiding this comment

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

@AlanCoding @jctanner This file should contain the set of permissions that we actually care about for enforcement. The problem is that pulp/django ship with a huge pile of random permissions, most of which we don't care about. galaxy.view_namespaces is a good example. Namespaces are always readable by everyone who is authenticated. We don't want to expose that permission to the UI or to users because adding it to a role does nothing.

@AlanCoding this is why I was so against using django permissions for DAB RBAC. It's practically impossible to get a full list of permissions that are actually enforced and actually do something in the system.

Copy link
Member

Choose a reason for hiding this comment

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

This led to me file ansible/django-ansible-base#457. But it should be super minor, I want to declare a change is probably necessary in DAB RBAC for the documented reasons, but I'm completely confident that won't be challenging to do. We've just haven't done it yet.

No-Issue

Signed-off-by: James Tanner <[email protected]>
@@ -182,18 +200,36 @@ class DeploymentMode(enum.Enum):
),
"ui_category": _("Ansible Repository"),
},
"container.view_containernamespace": {
Copy link
Member

Choose a reason for hiding this comment

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

A lot of the permissions you've added here don't actually do anything. Namespaces, for example, are viewable to everyone who is authenticated. We don't have the notion of a private namespace. I'd like to keep this file limited to the set of permissions that are actually enforced.

I think these will start to make sense once we fully integrate organizations, but for now this will just make the UI confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need a discrete list of what's needed in that case. I had to write a script to find the missing "read" perms for each category because there's no pointers otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

This file is the discrete list of what's needed. That's why I sent it to you. These are the permissions we care about.

"ansible.delete_collection": {
"name": _("Delete collection"),
"object_description": _("Delete this collection."),
"global_description": _("Delete any existing collection."),
"ui_category": _("Collections"),
},
# "ansible.view_ansiblerepository": {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you comment this one out? We do support private repositories, so view permissions here should matter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because this data structure is a dictionary and the key can only have one instance, which makes it impossible to add the same key to multiple categories.

Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be added to multiple categories?

@jctanner jctanner requested a review from newswangerd June 11, 2024 13:46
@jctanner jctanner mentioned this pull request Jun 11, 2024
@jctanner
Copy link
Collaborator Author

replaced by #2168

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.

4 participants