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

Fix to show meaningful policy name when add/delete it in audit.log #4017

Merged
merged 4 commits into from
Jun 22, 2018

Conversation

hsong-rh
Copy link
Contributor

@hsong-rh hsong-rh commented May 31, 2018

Show meaningful policy name in audit.log when adding/deleting policy.

https://bugzilla.redhat.com/show_bug.cgi?id=1434762

Steps for Testing/QA

  1. In UI, Control->Policies, add or delete a policy;
  2. [policy_name] Record added [deleted] should show in the audit.log

Related PR:
ManageIQ/manageiq#17504

@hsong-rh
Copy link
Contributor Author

@dclarizio @roliveri Please review.

@@ -116,6 +116,7 @@ def process_elements(elements, klass, task, display_name = nil, order_field = ni
Rbac.filtered(klass.where(:id => elements).order(order_by)).each do |record|
name = record.send(order_field.to_sym)
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved under the else below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is only for the request of policy deletion:

Delete a policy
---------------
CloudForms logs the following when a policy is deleted from the VMDB:
[----] I, [2017-03-03T13:26:34.193211] INFO -- : <AuditSuccess> MIQ(CiProcessing.process_element_destroy) userid: [test-user] - 
[0e9e8c56-ffab-11e6-8359-001a4aa0a7d6] Record deleted.

Request that the audit line is amended to include the following:
- Name of the policy

@@ -942,6 +942,8 @@ def folder_get_info(folder_node)
# Build the audit object when a profile is saved
def build_saved_audit(record, add = false)
name = record.respond_to?(:name) ? record.name : record.description
name = record.description if record.kind_of?(MiqPolicy)
Copy link
Member

Choose a reason for hiding this comment

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

The conditionals for setting name seem a bit clumsy. It's not a big deal, but there's probably a cleaner way to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will change it as:

    name = if record.kind_of?(MiqPolicy)
             record.description
           else
             record.respond_to?(:name) ? record.name : record.description
           end

@miq-bot
Copy link
Member

miq-bot commented Jun 4, 2018

Checked commits hsong-rh/manageiq-ui-classic@1ef0b9d~...6d117b0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@hsong-rh
Copy link
Contributor Author

@martinpovolny Can you review this? thanks.

@martinpovolny martinpovolny merged commit 21be641 into ManageIQ:master Jun 22, 2018
@martinpovolny
Copy link
Member

Merging this to close the BZ. However we should solve these issues with decorators.

Ping @skateman

@martinpovolny martinpovolny added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 22, 2018
@skateman
Copy link
Member

@martinpovolny I don't really understand the context here 😕

@martinpovolny
Copy link
Member

@skateman : I wanted to point you to yet another place that needs the decorators. Thinking you sort of oversee that part.

@skateman
Copy link
Member

I still don't understand what should be here in what decorator ...

@martinpovolny
Copy link
Member

 name = record.send(:description) if klass.name == 'MiqPolicy'

and

 name = if record.kind_of?(MiqPolicy)
             record.description
           else
             record.respond_to?(:name) ? record.name : record.description
           end

So we have some "display name" here that sometimes is "name" and in other cases it's "description"

We ask the record "who are you?" we don't tell it "gimme your display name!" And we do this on 2 places in the controller code (who knows where else we have the same problem).

And it's unlikely that the core would include a method to fix this.

Still does not look like a case for decorator? @skateman? are you awake? :-D

@skateman
Copy link
Member

Yup, makes sense now.

@hsong-rh hsong-rh deleted the audit_event_log branch June 26, 2018 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants