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

Log when User/SA/Roles is added to a group #1205

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0d285c3
Log when a user/sa/role is added to a group
Ellen-Yi-Dong Sep 18, 2024
f177dfe
Update format and add unit tests
Ellen-Yi-Dong Sep 19, 2024
52d2425
Update syntax error
Ellen-Yi-Dong Sep 19, 2024
1879794
Adding filtering to better view the audit logs
Ellen-Yi-Dong Sep 19, 2024
e7f1634
Update better syntax to description (thank you Petra again!)
Ellen-Yi-Dong Sep 19, 2024
695f3d5
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Sep 23, 2024
98ec671
Adding changes based on comments from PR
Ellen-Yi-Dong Sep 23, 2024
7d1b53c
Update type to TYPE_SERVICE_ACCOUNT based on comments
Ellen-Yi-Dong Sep 23, 2024
a2b27d7
Merge branch 'master' into log_add
Ellen-Yi-Dong Sep 25, 2024
1082105
Remove migration for now from conflicts
Ellen-Yi-Dong Sep 25, 2024
c4f2149
Adding changes based on comments
Ellen-Yi-Dong Sep 25, 2024
d93b2f5
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Sep 26, 2024
ed8c1fd
Update according to recent rebase
Ellen-Yi-Dong Sep 26, 2024
2728973
Update error type according to changes
Ellen-Yi-Dong Sep 26, 2024
b90c810
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 1, 2024
6c86869
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 11, 2024
7a56c67
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Oct 15, 2024
db76f34
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Oct 15, 2024
2102012
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Oct 15, 2024
fc2a1a1
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Oct 15, 2024
9170246
Update PR related to recent updates and comments
Ellen-Yi-Dong Oct 15, 2024
8c5b494
Update pre-commit file with new revision 24.10.0
Ellen-Yi-Dong Oct 15, 2024
12754a5
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 16, 2024
33592e0
Update PR with related comments
Ellen-Yi-Dong Oct 16, 2024
559bb7a
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 17, 2024
c3316c0
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 18, 2024
2a3d3f8
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 23, 2024
d87cf2d
Merge branch 'master' into log_add
Ellen-Yi-Dong Oct 24, 2024
0f47ea5
Merge branch 'master' into log_add
Ellen-Yi-Dong Jan 6, 2025
34de838
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Jan 8, 2025
696480a
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Jan 14, 2025
5b0ab17
Update changes to audit logs to fit the latest changes and after rebase
Ellen-Yi-Dong Jan 14, 2025
c47931f
Merge branch 'master' into log_add
Ellen-Yi-Dong Jan 14, 2025
8e4504b
Merge branch 'master' into log_add
Ellen-Yi-Dong Jan 20, 2025
8ff3678
Merge branch 'master' into log_add
lpichler Jan 21, 2025
a3c06b6
Merge branch 'master' into log_add
Ellen-Yi-Dong Jan 21, 2025
a1d5e22
Merge branch 'RedHatInsights:master' into log_add
Ellen-Yi-Dong Jan 29, 2025
d421e47
Merge branch 'master' into log_add
Ellen-Yi-Dong Jan 30, 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
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ repos:
pass_filenames: false
require_serial: true
- repo: https://github.com/psf/black
rev: 24.8.0
rev: 24.10.0
hooks:
- id: black
args: ["--check", "-l", "119", "-t", "py39", "rbac", "tests", "--diff"]
Expand Down
35 changes: 32 additions & 3 deletions rbac/management/audit_log/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,8 @@ def get_resource_item(self, r_type, request, *args, **kwargs):
group_object_name = "group: " + group_object.name
return group_object_id, group_object_name

elif r_type == AuditLog.PERMISSION:
# TODO: update for permission related items
return None
else:
return ValueError("Wrong Resource Type")

def find_edited_field(self, resource, resource_name, request, object):
"""Add additional information when group/role is edited."""
Expand All @@ -98,6 +97,22 @@ def find_edited_field(self, resource, resource_name, request, object):
description = description + "edited access (permissions/resources)"
return description

def find_specific_list_of_users(self, type_dict, user_type):
"""Create list of principals/roles/service accounts for description."""
names_list = []
if user_type == "user" or user_type == "User":
for username in type_dict:
names_list.append(username["username"])
elif user_type == "service_account" or user_type == "Service Account":
for service_account in type_dict:
names_list.append(service_account["clientId"])
elif user_type == AuditLog.ROLE:
for role in type_dict:
names_list.append(str(role.uuid))
else:
return ValueError("User type does not exist")
return ", ".join(names_list)

def log_create(self, request, resource):
"""Audit Log when a role or a group is created."""
self.principal_username = request.user.username
Expand Down Expand Up @@ -138,3 +153,17 @@ def log_edit(self, request, resource, object):
self.action = AuditLog.EDIT
self.tenant_id = self.get_tenant_id(request)
super(AuditLog, self).save()

def log_group_assignment(self, request, resource, object, type_dict, user_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

resource and object names are confusing

it looks that resource is rather resource_type and then object can be resource :)

Does it make sense ?

Copy link
Contributor

Choose a reason for hiding this comment

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

user_type could be user, service or role. What about name target_resource_type or assigned_resource_type ? or something like that

"""Audit Log when a role, user/principal, or service account is added to a group."""
self.principal_username = request.user.username
self.resource_type = resource
self.resource_id = object.id
resource_name = "group: " + object.name

name_list = self.find_specific_list_of_users(type_dict, user_type)
self.description = f"{user_type} {name_list} added to {resource_name}"

self.action = AuditLog.ADD
self.tenant_id = self.get_tenant_id(request)
super(AuditLog, self).save()
6 changes: 6 additions & 0 deletions rbac/management/audit_log/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ class AuditLogViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
serializer_class = AuditLogSerializer
permission_classes = (AuditLogAccessPermission,)

def order_by_id(self, queryset):
"""Order queryset by id."""
return queryset.order_by("-id").values()

def list(self, request, *args, **kwargs):
"""List all of the audit logs within database by tenant."""
self.queryset = filter_queryset_by_tenant(AuditLog.objects.all(), request.tenant)
if request.query_params.get("order_by") is not None:
self.queryset = self.order_by_id(self.queryset)
return super().list(request=request, args=args, kwargs=kwargs)
3 changes: 3 additions & 0 deletions rbac/management/group/definer.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ def add_roles(group, roles_or_role_ids, tenant, user=None):
"""Process list of roles and add them to the group."""
roles = _roles_by_query_or_ids(roles_or_role_ids)
group_name = group.name
auditlog_roles = []

group, created = Group.objects.get_or_create(name=group_name, tenant=tenant)
system_policy_name = "System Policy for Group {}".format(group.uuid)
Expand Down Expand Up @@ -172,6 +173,8 @@ def add_roles(group, roles_or_role_ids, tenant, user=None):
dual_write_handler.replicate_added_role(role)
# Send notifications
group_role_change_notification_handler(user, group, role, "added")
auditlog_roles.append(role)
return auditlog_roles


@transaction.atomic
Expand Down
101 changes: 89 additions & 12 deletions rbac/management/group/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,11 @@
from management.principal.model import Principal
from management.principal.proxy import PrincipalProxy
from management.principal.serializer import ServiceAccountSerializer
from management.principal.view import ADMIN_ONLY_KEY, USERNAME_ONLY_KEY, VALID_BOOLEAN_VALUE
from management.principal.view import (
ADMIN_ONLY_KEY,
USERNAME_ONLY_KEY,
VALID_BOOLEAN_VALUE,
)
from management.querysets import get_group_queryset, get_role_queryset
from management.relation_replicator.relation_replicator import ReplicationEventType
from management.role.view import RoleViewSet
Expand Down Expand Up @@ -88,7 +92,12 @@
SERVICE_ACCOUNT_NAME_KEY = "service_account_name"
SERVICE_ACCOUNT_USERNAME_FORMAT = "service-account-{clientId}"
VALID_EXCLUDE_VALUES = ["true", "false"]
VALID_GROUP_ROLE_FILTERS = ["role_name", "role_description", "role_display_name", "role_system"]
VALID_GROUP_ROLE_FILTERS = [
"role_name",
"role_description",
"role_display_name",
"role_system",
]
VALID_GROUP_PRINCIPAL_FILTERS = ["principal_username"]
VALID_PRINCIPAL_ORDER_FIELDS = ["username"]
VALID_PRINCIPAL_TYPE_VALUE = ["service-account", "user"]
Expand Down Expand Up @@ -116,7 +125,10 @@ def roles_filter(self, queryset, field, values):
roles_list = [value.lower() for value in values.split(",")]

discriminator = validate_and_get_key(
self.request.query_params, ROLE_DISCRIMINATOR_KEY, VALID_ROLE_ROLE_DISCRIMINATOR, "any"
self.request.query_params,
ROLE_DISCRIMINATOR_KEY,
VALID_ROLE_ROLE_DISCRIMINATOR,
"any",
)

if discriminator == "any":
Expand Down Expand Up @@ -432,7 +444,13 @@ def validate_principals_in_proxy_request(self, principals, org_id=None):
if len(resp.get("data", [])) == 0:
return {
"status_code": status.HTTP_404_NOT_FOUND,
"errors": [{"detail": "User(s) {} not found.".format(users), "status": "404", "source": "principals"}],
"errors": [
{
"detail": "User(s) {} not found.".format(users),
"status": "404",
"source": "principals",
}
],
}
return resp

Expand Down Expand Up @@ -689,15 +707,25 @@ def principals(self, request: Request, uuid: Optional[UUID] = None):
return Response(
status=status.HTTP_403_FORBIDDEN,
data={
"errors": [{"detail": str(ipe), "status": status.HTTP_403_FORBIDDEN, "source": "groups"}]
"errors": [
{
"detail": str(ipe),
"status": status.HTTP_403_FORBIDDEN,
"source": "groups",
}
]
},
)
except ServiceAccountNotFoundError as sanfe:
return Response(
status=status.HTTP_400_BAD_REQUEST,
data={
"errors": [
{"detail": str(sanfe), "source": "group", "status": str(status.HTTP_400_BAD_REQUEST)}
{
"detail": str(sanfe),
"source": "group",
"status": str(status.HTTP_400_BAD_REQUEST),
}
]
},
)
Expand Down Expand Up @@ -731,14 +759,39 @@ def principals(self, request: Request, uuid: Optional[UUID] = None):
# Serialize the group...
output = GroupSerializer(group)
response = Response(status=status.HTTP_200_OK, data=output.data)

if status.is_success(response.status_code):
if len(principals) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be part of transaction block which start on 715.
you can put audit log creation under those related conditions (len(service_accounts|principals) > 0).

if operation fails, transaction is rolled back and record into audit log is not created.

auditlog = AuditLog()
auditlog.log_group_assignment(
request,
AuditLog.GROUP,
object=group,
type_dict=principals,
user_type=Principal.Types.USER,
)
if len(service_accounts) > 0:
auditlog = AuditLog()
auditlog.log_group_assignment(
request,
AuditLog.GROUP,
object=group,
type_dict=service_accounts,
user_type=Principal.Types.SERVICE_ACCOUNT,
)

elif request.method == "GET":
# Check if the request comes with a bunch of service account client IDs that we need to check. Since this
# query parameter is incompatible with any other query parameter, we make the checks first. That way if any
# other query parameter was specified, we simply return early.
if SERVICE_ACCOUNT_CLIENT_IDS_KEY in request.query_params:
# pagination is ignored in this case
for query_param in request.query_params:
if query_param not in [SERVICE_ACCOUNT_CLIENT_IDS_KEY, "limit", "offset"]:
if query_param not in [
SERVICE_ACCOUNT_CLIENT_IDS_KEY,
"limit",
"offset",
]:
return Response(
status=status.HTTP_400_BAD_REQUEST,
data={
Expand Down Expand Up @@ -817,7 +870,11 @@ def principals(self, request: Request, uuid: Optional[UUID] = None):

# Get the "username_only" query parameter.
username_only = validate_and_get_key(
request.query_params, USERNAME_ONLY_KEY, VALID_BOOLEAN_VALUE, "false", required=False
request.query_params,
USERNAME_ONLY_KEY,
VALID_BOOLEAN_VALUE,
"false",
required=False,
)

# Build the options dict.
Expand All @@ -827,7 +884,10 @@ def principals(self, request: Request, uuid: Optional[UUID] = None):
# parameter. It is important because we need to call BOP for
# the users, and IT for the service accounts.
principalType = validate_and_get_key(
request.query_params, PRINCIPAL_TYPE_KEY, VALID_PRINCIPAL_TYPE_VALUE, required=False
request.query_params,
PRINCIPAL_TYPE_KEY,
VALID_PRINCIPAL_TYPE_VALUE,
required=False,
)

# Store the principal type in the options dict.
Expand Down Expand Up @@ -856,7 +916,10 @@ def principals(self, request: Request, uuid: Optional[UUID] = None):
service_accounts = it_service.get_service_accounts_group(
group=group, user=request.user, options=options
)
except (requests.exceptions.ConnectionError, UnexpectedStatusCodeFromITError):
except (
requests.exceptions.ConnectionError,
UnexpectedStatusCodeFromITError,
):
return Response(
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
data={
Expand Down Expand Up @@ -1041,8 +1104,19 @@ def roles(self, request, uuid=None, principals=None):
if serializer.is_valid(raise_exception=True):
roles = request.data.pop(ROLES_KEY, [])
group = set_system_flag_before_update(group, request.tenant, request.user)
add_roles(group, roles, request.tenant, user=request.user)
log_roles = add_roles(group, roles, request.tenant, user=request.user)
response_data = GroupRoleSerializerIn(group)
response = Response(status=status.HTTP_200_OK, data=response_data.data)
if status.is_success(response.status_code):
auditlog = AuditLog()
auditlog.log_group_assignment(
request,
AuditLog.GROUP,
object=group,
type_dict=log_roles,
user_type=AuditLog.ROLE,
)

elif request.method == "GET":
serialized_roles = self.obtain_roles(request, group)
page = self.paginate_queryset(serialized_roles)
Expand Down Expand Up @@ -1142,7 +1216,10 @@ def remove_service_accounts(self, user: User, group: Group, service_accounts: It

# Get the group's service accounts that match the service accounts that the user specified.
valid_service_accounts = Principal.objects.filter(
group=group, tenant=tenant, type="service-account", service_account_id__in=service_accounts
group=group,
tenant=tenant,
type=Principal.Types.SERVICE_ACCOUNT,
service_account_id__in=service_accounts,
)

# Collect the service account IDs the user specified.
Expand Down
Loading