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)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.2
People
(Reporter: mkanat, Assigned: mkanat)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
6.07 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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 | ||
Comment 2•13 years ago
|
||
Comment on attachment 501137 [details] [diff] [review] v1 Oh wait, this doesn't track which object was updated.
Attachment #501137 -
Flags: review?(LpSolit)
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #501137 -
Attachment is obsolete: true
Attachment #501145 -
Flags: review?(dkl)
Comment 4•13 years ago
|
||
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-
Assignee | ||
Comment 5•13 years ago
|
||
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)
Comment 6•13 years ago
|
||
(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 7•13 years ago
|
||
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-
Assignee | ||
Comment 8•13 years ago
|
||
(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?
Comment 9•13 years ago
|
||
(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
Comment 10•13 years ago
|
||
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
Assignee | ||
Comment 11•13 years ago
|
||
(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.
Assignee | ||
Comment 12•13 years ago
|
||
Okay, this fixes the bug that you pointed out.
Attachment #501155 -
Attachment is obsolete: true
Attachment #516219 -
Flags: review?(dkl)
Comment 13•13 years ago
|
||
Comment on attachment 516219 [details] [diff] [review] v4 Looks good. r=dkl
Attachment #516219 -
Flags: review?(dkl) → review+
Updated•13 years ago
|
Flags: approval?
Assignee | ||
Updated•13 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 14•13 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•