-
Notifications
You must be signed in to change notification settings - Fork 134
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
system auditor role #2162
Conversation
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]>
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]>
@@ -30,6 +30,12 @@ class DeploymentMode(enum.Enum): | |||
# Category to group the permission in the UI. | |||
"ui_category": _("Collection Namespaces"), | |||
}, | |||
"galaxy.view_namespace": { |
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.
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?
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.
@AlanCoding I was pointed to this file just last night and I don't fully know how it works.
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.
@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.
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.
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": { |
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.
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.
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 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.
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.
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": { |
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.
Why did you comment this one out? We do support private repositories, so view permissions here should matter.
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.
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.
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.
Why does this need to be added to multiple categories?
replaced by #2168 |
https://issues.redhat.com/browse/AAP-24730
replaces #2157