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.

Sign up for GitHub

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

system auditor role #2162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b83c60f
Stuff.
jctanner Jun 10, 2024
1ab20a3
More stuff.
jctanner Jun 10, 2024
a02c99b
Checkin.
jctanner Jun 10, 2024
b40beed
Checkin.
jctanner Jun 10, 2024
3888a48
Checkin.
jctanner Jun 10, 2024
9dd563c
Add the role.
jctanner Jun 10, 2024
b1189c7
Bypass lint complaints?
jctanner Jun 10, 2024
3032afd
Add integration test from other PR.
jctanner Jun 10, 2024
0bbf8e0
Try to fix unit test.
jctanner Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions galaxy_ng/app/access_control/statements/roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,26 @@
},
"inherit_from": [],
},
# View anything but not add/edit/delete.
"galaxy.auditor": {
"permissions": {
"ansible.view_ansiblerepository",
"ansible.view_collection",
"ansible.view_collectionremote",
"auth.view_group",
"container.namespace_view_containerdistribution",
"container.view_containernamespace",
"container.view_containerrepository",
"core.view_group",
"core.view_task",
"galaxy.view_containernamespace",
"galaxy.view_containerregistryremote",
"galaxy.view_group",
"galaxy.view_namespace",
"galaxy.view_user",
},
"inherit_from": []
},
}


Expand Down
48 changes: 48 additions & 0 deletions galaxy_ng/app/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"name": _("View namespace"),
"object_description": _("View this namespace."),
"global_description": _("View any existing namespace."),
"ui_category": _("Collection Namespaces"),
},
"galaxy.change_namespace": {
"name": _("Change namespace"),
"object_description": _("Edit this namespace."),
Expand All @@ -48,12 +54,24 @@ class DeploymentMode(enum.Enum):
"global_description": _("Upload collections to any existing namespace."),
"ui_category": _("Collection Namespaces"),
},
"ansible.view_collection": {
"name": _("View collection"),
"object_description": _("View this collection."),
"global_description": _("View any existing collection."),
"ui_category": _("Collections"),
},
"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?

# "name": _("View Ansible repository"),
# "object_description": _("View this Ansible repository."),
# "global_description": _("View this Ansible repository."),
# "ui_category": _("Collections"),
# },
"ansible.modify_ansible_repo_content": {
"name": _("Modify Ansible repo content"),
"object_description": _("Modify content of this Ansible repository."),
Expand Down Expand Up @@ -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.

"name": _("View container namespace permissions"),
"object_description": _("View permissions on this container namespace."),
"global_description": _("View permissions on any existing container namespace."),
"ui_category": _("Execution Environments"),
},
"container.change_containernamespace": {
"name": _("Change container namespace permissions"),
"object_description": _("Edit permissions on this container namespace."),
"global_description": _("Edit permissions on any existing container namespace."),
"ui_category": _("Execution Environments"),
},
"container.namespace_view_containerdistribution": {
"name": _("View containers"),
"object_description": _("View all objects in this container namespace."),
"global_description": _("View all objects in any container namespace in the system."),
"ui_category": _("Execution Environments"),
},
"container.namespace_change_containerdistribution": {
"name": _("Change containers"),
"object_description": _("Edit all objects in this container namespace."),
"global_description": _("Edit all objects in any container namespace in the system."),
"ui_category": _("Execution Environments"),
},
"container.namespace_view_content_containerpushrepository" : {
"name": _("View image tags"),
"object_description": _("View an image's tag in this container namespace"),
"global_description": _("View an image's tag in any container namespace the system."),
"ui_category": _("Execution Environments"),
},
"container.namespace_modify_content_containerpushrepository" : {
"name": _("Change image tags"),
"object_description": _("Edit an image's tag in this container namespace"),
Expand All @@ -206,6 +242,12 @@ class DeploymentMode(enum.Enum):
"global_description": _("Add new containers to the system."),
"ui_category": _("Execution Environments"),
},
"container.view_containerrepository": {
"name": _("View container repository"),
"object_description": _("View this container repository."),
"global_description": _("View any existing container repository in the system."),
"ui_category": _("Execution Environments"),
},
"container.delete_containerrepository": {
"name": _("Delete container repository"),
"object_description": _("Delete this container repository."),
Expand All @@ -230,6 +272,12 @@ class DeploymentMode(enum.Enum):
"global_description": _("Manage container namespace roles existing in the system."),
"ui_category": _("Execution Environments"),
},
"galaxy.view_containerregistryremote": {
"name": _("View remote registry"),
"object_description": None,
"global_description": _("View remote registries in the system."),
"ui_category": _("Container Registry Remotes"),
},
"galaxy.add_containerregistryremote": {
"name": _("Add remote registry"),
"object_description": None,
Expand Down
39 changes: 39 additions & 0 deletions galaxy_ng/tests/integration/api/test_system_auditor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import json
import os
import uuid

import pytest


pytestmark = pytest.mark.qa # noqa: F821


@pytest.mark.deployment_standalone
@pytest.mark.min_hub_version("4.10dev")
@pytest.mark.skipif(
os.getenv("ENABLE_DAB_TESTS"),
reason="Skipping test because ENABLE_DAB_TESTS is set"
)
def test_system_auditor_role_permissions_without_gateway(galaxy_client):
"""Tests the galaxy.system_auditor role can be added to a user and has the right perms."""

gc = galaxy_client("admin", ignore_cache=True)

# make a random user
username = str(uuid.uuid4())
uinfo = gc.post(
"_ui/v1/users/",
body=json.dumps({"username": username, "password": "redhat1234"})
)
uid = uinfo["id"]

# assign the galaxy.system_auditor role to the user
rinfo = gc.post(
f"pulp/api/v3/users/{uid}/roles/",
body=json.dumps({'content_object': None, 'role': 'galaxy.auditor'})
)

# check that all the permssions are view_* only ...
for perm_code in rinfo["permissions"]:
perm_name = perm_code.split(".", 1)[1]
assert "view_" in perm_name, f"{perm_code} is not a view-only permission"
5 changes: 4 additions & 1 deletion galaxy_ng/tests/unit/app/test_role_permissions_in_constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ def test_permissions_defined_in_roles_have_description(self):
'galaxy.delete_synclist',
'galaxy.view_synclist',
'galaxy.add_synclist',
'galaxy.change_synclist'
'galaxy.change_synclist',
'galaxy.view_containernamespace',
'core.view_group',
'auth.view_group',
}

constant_permissions = constant_permissions.union(ignored_permissions)
Expand Down
Loading