Closed Bug 622943 Opened 13 years ago Closed 13 years ago

Simple auditing of all changes to all Bugzilla::Object objects

Categories

(Bugzilla :: Bugzilla-General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: mkanat, Assigned: mkanat)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bugzilla could use some better auditing of user actions. The simplest thing to start with is implementing a simple auditing for all the basic fields changed by Bugzilla::Object->update, and also noting when an object is created.
Attached patch v1 (obsolete) — Splinter Review
This is *remarkably* simple. It doesn't present a user interface yet, just tracks the data in a table that people can access in MySQL if they need to. It doesn't track everything, just the basic DB_COLUMNS fields for every object.

The reason that user_id can be NULL is for scripts like checksetup and migrate.pl that call update() and create() methods with no logged-in user.

Adding a UI and tracking more stuff can happen in later patches.
Assignee: general → mkanat
Status: NEW → ASSIGNED
Attachment #501137 - Flags: review?(LpSolit)
Comment on attachment 501137 [details] [diff] [review]
v1

Oh wait, this doesn't track which object was updated.
Attachment #501137 - Flags: review?(LpSolit)
Attached patch v2 (obsolete) — Splinter Review
Attachment #501137 - Attachment is obsolete: true
Attachment #501145 - Flags: review?(dkl)
Blocks: 562505
Comment on attachment 501145 [details] [diff] [review]
v2

There is no valid reason to audit changes made outside admin pages, e.g. when creating/editing my saved searches. Also, your code will duplicate data about attachments and flags, which call Bugzilla::Object.
Attachment #501145 - Flags: review-
Attached patch v3 (obsolete) — Splinter Review
I'm not sure exactly what your suggestion is related to, about admin pages. Tracking any changes or creations for an object is valuable.

This prevents audit_log from tracking attachments and flags.
Attachment #501145 - Attachment is obsolete: true
Attachment #501155 - Flags: review?(dkl)
Attachment #501145 - Flags: review?(dkl)
(In reply to comment #5)
> I'm not sure exactly what your suggestion is related to, about admin pages.

I mean that logging all changes made in admin pages (all the edit*.cgi pages) make sense, but not those done outside these pages, e.g. nobody cares when I edit my saved searches, or add one to the footer of my pages.


> Tracking any changes or creations for an object is valuable.

Not the ones which are personal, as in my example above. That's my strong objection.
Comment on attachment 501155 [details] [diff] [review]
v3

>+    # During update, it's the actual %changes hash produced by update().
>+    foreach my $field (keys %$changes) {
>+        my ($from, $to) = @{ $changes->{$field} };
>+        $sth->execute($user_id, $self->id, $class, $field, $from, $to);
>+    }
>+}

Need to reverse $self->id and $class:

        $sth->execute($user_id, $class, $self->id, $field, $from, $to);

Couple comments:

1. All in all it works well except for the modules that do not call Bugzilla::Object->remove_from_db and Bugzilla::Object->create.
Some examples are Component.pm and Product.pm. For example deleting a product or component is not logged.

2. Some modules have their own update() which does call Bugzilla::Object->update but also have other code around it for special
purposes. Once example is updating the initial cc list for a component, which is not logged.

3. When changing the initial owner for a component, the user id is stored instead of the user's login. Should we 
store the login instead of the user id? 

Otherwise for the most part this looks good code wise and works for the modules that use the standard Bugzilla::Object calls. As for
the "should we do this at all?" debate, I will leave that for you others to decide.

Dave
Attachment #501155 - Flags: review?(dkl) → review-
(In reply to comment #7)
> 1. All in all it works well except for the modules that do not call
> Bugzilla::Object->remove_from_db and Bugzilla::Object->create.
> Some examples are Component.pm and Product.pm. For example deleting a product
> or component is not logged.

  Yep. That's some stuff to handle in a follow-up bug.

> 2. Some modules have their own update() which does call
> Bugzilla::Object->update but also have other code around it for special
> purposes. Once example is updating the initial cc list for a component, which
> is not logged.

  Yeah, that's all stuff to handle in various follow-up bugs. :-)

> 3. When changing the initial owner for a component, the user id is stored
> instead of the user's login. Should we 
> store the login instead of the user id? 

  Ah, no, I think it's okay to store the actual literal values, particularly because the username might change over time and then you have no idea who that was when you're looking at the audit table.

> As for
> the "should we do this at all?" debate, I will leave that for you others to
> decide.

  Okay. Although I would like your feedback--is this something that you would have wanted at Red Hat, and is it something that you want at Mozilla?
(In reply to comment #8)
>   Okay. Although I would like your feedback--is this something that you would
> have wanted at Red Hat, and is it something that you want at Mozilla?

At Red Hat, my old Boss would jump all over this as we have had several instances where it would have been nice to have known who made a specific admin change. We had people adding/editing flags that messed up things for other groups and we could never tell who exactly did it. Or who added a component to a product that they should not have been messing with. Things like that. So for Red Hat it would be a nice feature, it would have to be back ported to 3.6 of course.

For Mozilla, I have only been here a short amount of time and are still gauging interest in various features, but maybe justdave, gerv, or reed can comment more on whether this would be helpful for Mozilla.org.

Dave
The things we care about auditing most at Mozilla are probably security bug accesses - and that wouldn't be covered by this, as it doesn't involve changing anything. (We have other mechanisms, external to Bugzilla, for auditing that.) 

It seems quite rare here that someone with admin privs screws things up and then refuses to admit to it... :-)

Gerv
(In reply to comment #10)
> It seems quite rare here that someone with admin privs screws things up and
> then refuses to admit to it... :-)

  It has happened before, and I've had specific requests from the people who are actually administering bmo that would have been solved by the existence of auditing. There are lots of other security mechanisms that become unnecessary--and others that become possible--with proper auditing.

  Auditing isn't about what you think *will* happen--it's about figuring out what *did* happen after something unexpected occurs.
Blocks: 100090
Attached patch v4Splinter Review
Okay, this fixes the bug that you pointed out.
Attachment #501155 - Attachment is obsolete: true
Attachment #516219 - Flags: review?(dkl)
Comment on attachment 516219 [details] [diff] [review]
v4

Looks good. r=dkl
Attachment #516219 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval? → approval+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Attachment.pm
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Constants.pm
modified Bugzilla/Flag.pm
modified Bugzilla/Object.pm
modified Bugzilla/DB/Schema.pm
Committed revision 7750.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: bz-audit
Blocks: 690173
Blocks: 715902
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: