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

Ellen-Yi-Dong · 2024-09-18T23:10:11Z

Link(s) to Jira

https://issues.redhat.com/browse/RHCLOUD-34891

Description of Intent of Change(s)

The what, why and how.

Log when a user/principal, service account, or role is added to a group

Local Testing

How can the feature be exercised?
How can the bug be exploited and fix confirmed?
Is any special local setup required?

Added unit tests to test changes

Checklist

if API spec changes are required, is the spec updated?
are there any pre/post merge actions required? if so, document here.
are theses changes covered by unit tests?
if warranted, are documentation changes accounted for?
does this require migration changes?
- if yes, are they backwards compatible?
is there known, direct impact to dependent teams/components?
- if yes, how will this be handled?

Secure Coding Practices Checklist Link

https://github.com/RedHatInsights/secure-coding-checklist

Secure Coding Practices Checklist

petracihalova

nice job 👍 I have few comments

rbac/management/audit_log/model.py

rbac/management/group/view.py

rbac/management/audit_log/view.py

astrozzc · 2024-09-23T21:17:26Z

rbac/management/group/view.py

-            group=group, tenant=tenant, type="service-account", service_account_id__in=service_accounts
+            group=group,
+            tenant=tenant,
+            type="service-account",


Let's use TYPE_SERVICE_ACCOUNT for this, I have a PR ongoing to replace these random type names with enums. So I can remember replacing this in my PR too

astrozzc

LGTM

lpichler · 2024-09-24T10:22:38Z

@astrozzc we should discuss when this PR can be merged as it conflicts with some dual write PR as #1198,..

lpichler · 2024-09-24T10:37:13Z

rbac/management/audit_log/model.py

@@ -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_add(self, request, resource, object, type_dict, user_type):


maybe better name ? it is not visible difference between log_add and log_create.
something like "log_role_or_user_added_to_group" or log_group_membership or log_group_assigment.

Oh! There's another task to add permissions to a role, so I was thinking of having this log_add function do that as well! I just have it for now grouped this way to split the prs. Do you think that's alright...?

astrozzc · 2024-09-24T13:15:58Z

@astrozzc we should discuss when this PR can be merged as it conflicts with some dual write PR as #1198,..

Oh, good point. That one is about to be merged, right? Let's merge that one first before we merge this one. That one is too big to do a rebase

lpichler · 2024-09-24T14:07:21Z

@astrozzc we should discuss when this PR can be merged as it conflicts with some dual write PR as #1198,..

Oh, good point. That one is about to be merged, right? Let's merge that one first before we merge this one. That one is too big to do a rebase

yes it is merged now but into #1198, this PR also contains migration. I am not sure what it is the best approach here.

if we will merge this, we would need to solve conflicts in #1198 and also change names of migrations.

astrozzc · 2024-10-16T18:09:41Z

rbac/management/audit_log/model.py

+        """Create list of principals/roles/service accounts for description."""
+        names_list = []
+        if user_type == "user" or user_type == "User":
+            for i in type_dict:


Nit: can we use a more meaningful name to replace these "i" here?
Such as principal_data, role_data, etc.

vbelchio

LGTM

But we need to solve the related bug below to have the best behavior of the audit logs and the process to add a non-existent role to a group.

JIRA Bug: https://issues.redhat.com/browse/RHCLOUD-35765

Ellen-Yi-Dong · 2024-10-23T14:11:37Z

/retest

lpichler · 2025-01-20T12:17:23Z

rbac/management/group/view.py

@@ -771,6 +785,27 @@ 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:


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.

lpichler · 2025-01-21T08:43:40Z

rbac/management/audit_log/model.py

@@ -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):


resource and object names are confusing

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

Does it make sense ?

lpichler · 2025-01-21T08:49:30Z

rbac/management/audit_log/model.py

@@ -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):


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

lpichler · 2025-01-21T09:00:45Z

rbac/management/group/view.py

+                    request,
+                    AuditLog.GROUP,
+                    object=group,
+                    type_dict=response_data.data,


what about to pass just values for description ? Something like:
[response_data.data["uuid"] for role in response_data.data]

then type_dict could be renamed to something like values_for_description
and it will allow to remove method self.find_specific_list_of_users and it is possible to join(",") in log assignment method.

Ellen-Yi-Dong added 2 commits September 18, 2024 16:09

Log when a user/sa/role is added to a group

0d285c3

Update format and add unit tests

f177dfe

Ellen-Yi-Dong changed the title ~~[WIP] Log when a user/sa/role is added to a group~~ Log when User/SA/Roles is added to a group Sep 19, 2024

Ellen-Yi-Dong added 2 commits September 18, 2024 20:46

Update syntax error

52d2425

Adding filtering to better view the audit logs

1879794

Ellen-Yi-Dong requested review from petracihalova, astrozzc, vbelchio, lpichler and dagbay-rh and removed request for astrozzc September 19, 2024 04:52

Update better syntax to description (thank you Petra again!)

e7f1634

petracihalova reviewed Sep 20, 2024

View reviewed changes

rbac/management/audit_log/model.py Outdated Show resolved Hide resolved

rbac/management/group/view.py Outdated Show resolved Hide resolved

rbac/management/audit_log/view.py Outdated Show resolved Hide resolved

rbac/management/audit_log/view.py Outdated Show resolved Hide resolved

Ellen-Yi-Dong added 2 commits September 23, 2024 13:24

Merge branch 'RedHatInsights:master' into log_add

695f3d5

Adding changes based on comments from PR

98ec671

astrozzc reviewed Sep 23, 2024

View reviewed changes

astrozzc approved these changes Sep 23, 2024

View reviewed changes

Update type to TYPE_SERVICE_ACCOUNT based on comments

7d1b53c

lpichler reviewed Sep 24, 2024

View reviewed changes

Merge branch 'master' into log_add

a2b27d7

Ellen-Yi-Dong force-pushed the log_add branch 2 times, most recently from f9465f2 to a2b27d7 Compare September 25, 2024 19:16

Remove migration for now from conflicts

1082105

Ellen-Yi-Dong requested review from lpichler and petracihalova September 25, 2024 19:33

Adding changes based on comments

c4f2149

Ellen-Yi-Dong added 6 commits October 15, 2024 10:36

Merge branch 'RedHatInsights:master' into log_add

7a56c67

Merge branch 'RedHatInsights:master' into log_add

db76f34

Merge branch 'RedHatInsights:master' into log_add

2102012

Merge branch 'RedHatInsights:master' into log_add

fc2a1a1

Update PR related to recent updates and comments

9170246

Update pre-commit file with new revision 24.10.0

8c5b494

astrozzc approved these changes Oct 16, 2024

View reviewed changes

Ellen-Yi-Dong added 2 commits October 16, 2024 12:21

Merge branch 'master' into log_add

12754a5

Update PR with related comments

33592e0

vbelchio approved these changes Oct 16, 2024

View reviewed changes

Ellen-Yi-Dong added 2 commits October 17, 2024 10:02

Merge branch 'master' into log_add

559bb7a

Merge branch 'master' into log_add

c3316c0

lpichler approved these changes Oct 22, 2024

View reviewed changes

Merge branch 'master' into log_add

2a3d3f8

Ellen-Yi-Dong added 6 commits October 24, 2024 08:04

Merge branch 'master' into log_add

d87cf2d

Merge branch 'master' into log_add

0f47ea5

Merge branch 'RedHatInsights:master' into log_add

34de838

Merge branch 'RedHatInsights:master' into log_add

696480a

Update changes to audit logs to fit the latest changes and after rebase

5b0ab17

Merge branch 'master' into log_add

c47931f

lpichler reviewed Jan 20, 2025

View reviewed changes

Ellen-Yi-Dong and others added 2 commits January 20, 2025 12:55

Merge branch 'master' into log_add

8e4504b

Merge branch 'master' into log_add

8ff3678

lpichler reviewed Jan 21, 2025

View reviewed changes

Ellen-Yi-Dong added 3 commits January 21, 2025 08:07

Merge branch 'master' into log_add

a3c06b6

Merge branch 'RedHatInsights:master' into log_add

a1d5e22

Merge branch 'master' into log_add

d421e47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Ellen-Yi-Dong commented Sep 18, 2024 •

edited

Loading

petracihalova left a comment

astrozzc Sep 23, 2024

astrozzc left a comment

lpichler commented Sep 24, 2024

lpichler Sep 24, 2024

Ellen-Yi-Dong Sep 24, 2024 •

edited

Loading

astrozzc commented Sep 24, 2024

lpichler commented Sep 24, 2024

astrozzc Oct 16, 2024

vbelchio left a comment

Ellen-Yi-Dong commented Oct 23, 2024

lpichler Jan 20, 2025

lpichler Jan 21, 2025

lpichler Jan 21, 2025

lpichler Jan 21, 2025

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

Are you sure you want to change the base?

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

Conversation

Ellen-Yi-Dong commented Sep 18, 2024 • edited Loading

Link(s) to Jira

Description of Intent of Change(s)

Local Testing

Checklist

Secure Coding Practices Checklist Link

Secure Coding Practices Checklist

petracihalova left a comment

Choose a reason for hiding this comment

astrozzc Sep 23, 2024

Choose a reason for hiding this comment

astrozzc left a comment

Choose a reason for hiding this comment

lpichler commented Sep 24, 2024

lpichler Sep 24, 2024

Choose a reason for hiding this comment

Ellen-Yi-Dong Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

astrozzc commented Sep 24, 2024

lpichler commented Sep 24, 2024

astrozzc Oct 16, 2024

Choose a reason for hiding this comment

vbelchio left a comment

Choose a reason for hiding this comment

Ellen-Yi-Dong commented Oct 23, 2024

lpichler Jan 20, 2025

Choose a reason for hiding this comment

lpichler Jan 21, 2025

Choose a reason for hiding this comment

lpichler Jan 21, 2025

Choose a reason for hiding this comment

lpichler Jan 21, 2025

Choose a reason for hiding this comment

Ellen-Yi-Dong commented Sep 18, 2024 •

edited

Loading

Ellen-Yi-Dong Sep 24, 2024 •

edited

Loading