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

[RHCLOUD-33414] - restrict the Custom default access group renaming #1383

Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ad04075
[RHCLOUD-33414] - restrict the Custom default access group renaming
EvanCasey13 Dec 11, 2024
a8dd7dc
Updated to only check groups being updated which are platform default
EvanCasey13 Dec 11, 2024
9e22e48
changes to use group obj
EvanCasey13 Dec 11, 2024
5733798
group not exist and conditional check change
EvanCasey13 Dec 11, 2024
39c0763
Custom group restriction refactoring
EvanCasey13 Dec 12, 2024
a575fa5
Remove print statement from test
EvanCasey13 Dec 12, 2024
1398d95
readd description checks for custom group restriction
EvanCasey13 Dec 12, 2024
25d51b9
If condition update for restriction
EvanCasey13 Dec 12, 2024
8de306e
Moved custom default group check to update method of serializer and s…
EvanCasey13 Dec 12, 2024
f7141f7
Added quotes around group name being returned & removed brackets
EvanCasey13 Dec 20, 2024
32ac9c2
Merge branch 'master' into custom_default_access_restriction
EvanCasey13 Dec 20, 2024
dbe460b
Added restrict custom default function to group view
EvanCasey13 Jan 6, 2025
3306e39
Merge branch 'custom_default_access_restriction' of github.com:EvanCa…
EvanCasey13 Jan 6, 2025
2bc1ed7
Merge branch 'master' into custom_default_access_restriction
EvanCasey13 Jan 6, 2025
07aed75
Fixed tests
EvanCasey13 Jan 7, 2025
bfb9a1a
Merge branch 'master' into custom_default_access_restriction
EvanCasey13 Jan 7, 2025
286b032
Merge branch 'master' into custom_default_access_restriction
EvanCasey13 Jan 8, 2025
bd8ff86
only restrict the field of name and description and check request method
EvanCasey13 Jan 22, 2025
2049f6c
Merge branch 'custom_default_access_restriction' of github.com:EvanCa…
EvanCasey13 Jan 22, 2025
e20f0ed
Merge branch 'master' into custom_default_access_restriction
petracihalova Jan 29, 2025
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
14 changes: 14 additions & 0 deletions rbac/management/group/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,17 @@ def protect_system_groups(self, action, group=None):
error = {key: [_(message)]}
raise serializers.ValidationError(error)

def restrict_custom_default_group_renaming(self, request, group):
EvanCasey13 marked this conversation as resolved.
Show resolved Hide resolved
"""Restrict users from changing the name or description of the Custom default group."""
invalid_parameters = ["name", "description"]
if group.platform_default and request.method == "PUT":
invalid_fields = [field for field in invalid_parameters if field in request.data]
if invalid_fields:
key = "detail"
message = "Updating the name or description of 'Custom default group' is restricted"
error = {key: (message)}
raise serializers.ValidationError(error)

def protect_default_admin_group_roles(self, group):
"""Disallow default admin access roles from being updated."""
if group.admin_default:
Expand Down Expand Up @@ -438,9 +449,12 @@ def update(self, request, *args, **kwargs):
self.protect_system_groups("update")

group = self.get_object()

if not request.user.admin:
self.protect_group_with_user_access_admin_role(group.roles_with_access(), "update_group")

self.restrict_custom_default_group_renaming(request, group)

update_group = super().update(request=request, args=args, kwargs=kwargs)

if status.is_success(update_group.status_code):
Expand Down
11 changes: 11 additions & 0 deletions tests/management/group/test_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,7 @@ def test_update_group_success(self, send_kafka_message, mock_request):
url = reverse("v1_management:group-detail", kwargs={"uuid": self.group.uuid})
client = APIClient()
response = client.put(url, test_data, format="json", **self.headers)

self.assertEqual(response.status_code, status.HTTP_200_OK)

self.assertIsNotNone(response.data.get("uuid"))
Expand Down Expand Up @@ -808,6 +809,16 @@ def test_update_default_group(self):
response = client.put(url, test_data, format="json", **self.headers)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)

def test_update_custom_default_group(self):
"""Test that Custom default group is protected from updates"""
customDefGroup = Group(name="customDefGroup", platform_default=True, system=False, tenant=self.tenant)
customDefGroup.save()
url = reverse("v1_management:group-detail", kwargs={"uuid": customDefGroup.uuid})
test_data = {"name": "new_name" + "_updated", "description": "new_description" + "_updated"}
client = APIClient()
response = client.put(url, test_data, format="json", **self.headers)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)

Copy link
Contributor

Choose a reason for hiding this comment

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

nice job 👍 I have small nitpick ... I would like to check not only status code but the text of the err message to be 100% sure that rbac returns 400 from correct condition, for example something like this

 expected_err_message = "Updating the name or description of 'Custom default group' is restricted"
 self.assertEqual(response.data.get("errors")[0].get("detail"), expected_err_message)

you can add it if you want but for now we can merge this PR

def test_update_admin_default_group(self):
"""Test that admin_default groups are protected from updates"""
url = reverse("v1_management:group-detail", kwargs={"uuid": self.adminGroup.uuid})
Expand Down
Loading