Closed
Bug 740536
Opened 12 years ago
Closed 12 years ago
Webservice for maximum time of audit_log
Categories
(Bugzilla :: WebService, enhancement)
Bugzilla
WebService
Tracking
()
RESOLVED
FIXED
Bugzilla 4.4
People
(Reporter: Frank, Assigned: Frank)
References
Details
Attachments
(1 file, 4 obsolete files)
2.21 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
For Mylyn we plan to use Webservice as replacement of config.cgi. To reduce the network data bandwidth we need an way to know what was the last time of an administration change.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #610634 -
Flags: review?(dkl)
Comment 2•12 years ago
|
||
Comment on attachment 610634 [details] [diff] [review] patch V1 Review of attachment 610634 [details] [diff] [review]: ----------------------------------------------------------------- Overall it looks fine to me and works as expected. Might be better to call it last_audit_time IMO. r=dkl ::: Bugzilla/WebService/Bugzilla.pm @@ +114,4 @@ > }; > } > > +sub max_audit_time { Maybe last_audit_time would be a better name?
Attachment #610634 -
Flags: review?(dkl) → review+
Assignee | ||
Comment 3•12 years ago
|
||
rename to last_audit_time as requested in comment#2
Attachment #610634 -
Attachment is obsolete: true
Attachment #611147 -
Flags: review?(dkl)
Comment 4•12 years ago
|
||
This won't tell you the last time of an administrative change; it will tell you the last time pretty much any object at all in Bugzilla was changed (besides, perhaps, bugs, comments, and attachments). So if a user changes something about themselves, the time will be updated. I don't really see how this WebService is valuable in and of itself.
Comment 5•12 years ago
|
||
Comment on attachment 611147 [details] [diff] [review] patch V2 Review of attachment 611147 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. The pod change can be made at checkin. r=dkl ::: Bugzilla/WebService/Bugzilla.pm @@ +418,5 @@ > +=item B<Params> (none) > + > +=item B<Returns> > + > +A hash with a single item, C<max_audit_time>, that is the maximum of the s/max_audit_time/last_audit_time/
Attachment #611147 -
Flags: review?(dkl) → review+
Updated•12 years ago
|
Flags: approval?
Comment 6•12 years ago
|
||
FWIW, I think Frank's problem would be resolved much better by implementing some sort of etag support in the WebService as a whole or something similar to that as a part of Bugzilla.config which isn't implemented yet and would return information similar to what config.cgi returns now.
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Max Kanat-Alexander from comment #6) > FWIW, I think Frank's problem would be resolved much better by implementing > some sort of etag support in the WebService as a whole or something similar > to that as a part of Bugzilla.config which isn't implemented yet and would > return information similar to what config.cgi returns now. Yes, this is bug bug#504937 but because of bug#504937#c3 I want to implement this with more web service calls from mylyn. I thought that audit_log is only used for administration objects. Actual I did not find an user action that get logged in the adit_log table. Here what I have tried: * change user preferences (General Preferences, Email Preferences, Saved Searches and Account Information) * save/ update a search * add an Whining event What did I miss?
Comment 9•12 years ago
|
||
(In reply to Frank Becker from comment #8) > I thought that audit_log is only used for administration objects. It's used for anything that subclasses Bugzilla::Object. In the future it will log even more than it does now. > Actual I did not find an user action that get logged in the adit_log table. There may or may not be one at the moment. I think there is, but I'm not sure. There could certainly be one in the future. This is just a generic auditing system for all changes to everything in Bugzilla.
Assignee | ||
Comment 10•12 years ago
|
||
Whats about the following? The audit_log table has an class field. We can restrict the max only for an class or a list of classes. So we maybe use the following list: * Bugzilla::Field * Bugzilla::Product * Bugzilla::Component * Bugzilla::FlagType * Bugzilla::Classification * Bugzilla::Group * Bugzilla::Keyword * Bugzilla::Version * Bugzilla::Milestone Thoughts?
Comment 11•12 years ago
|
||
(In reply to Frank Becker from comment #10) > Whats about the following? > > The audit_log table has an class field. We can restrict the max only for an > class or a list of classes. > > So we maybe use the following list: > * Bugzilla::Field > * Bugzilla::Product > * Bugzilla::Component > * Bugzilla::FlagType > * Bugzilla::Classification > * Bugzilla::Group > * Bugzilla::Keyword > * Bugzilla::Version > * Bugzilla::Milestone > > Thoughts? I would be OK with having a 'class' parameter that has one or more values that filter the rows that are examined when determining last time. If 'class' is not passing it does the current behaviour of that patch. Objections? dkl
Assignee | ||
Comment 12•12 years ago
|
||
new version with parameter class
Attachment #611147 -
Attachment is obsolete: true
Attachment #612600 -
Flags: review?(dkl)
Updated•12 years ago
|
Flags: approval?
Comment 13•12 years ago
|
||
Comment on attachment 612600 [details] [diff] [review] patch V3 Review of attachment 612600 [details] [diff] [review]: ----------------------------------------------------------------- ::: Bugzilla/WebService/Bugzilla.pm @@ +129,5 @@ > + push(@$class_array, $class_temp); > + } > + if (defined $class_array) { > + my $attr = "'" . join ( '\',\'', @$class_array) . "'"; > + $sql_statment = "SELECT MAX(at_time) FROM audit_log WHERE class in ($attr)"; You can combine a lot of code to make shorter and remove duplication. You should also use $dbh->sql_in() and placeholders to protect against SQL injection vulnerabilities. my $sql_statement = "SELECT MAX(at_time) FROM audit_log"; my @class_values = map { trick_taint($_) } @{$param->{class}}; if (@class_values) { my @qmarks = ("?") x @class_values; $sql_statement .= " WHERE " . $dbh->sql_in('class', \@qmarks); } my $last_audit_time = $dbh->selectrow_array("$sql_statement", undef, @class_values); @@ +447,5 @@ > + > +=item C<class> (array) - An array of strings representing the class names. > + > +B<Note:> The class names are defined as "Bugzilla::<class_name>". For the product > +use "Bugzilla:Product". Bugzilla::Product @@ +454,5 @@ > + > +=item B<Returns> > + > +A hash with a single item, C<last_audit_time>, that is the maximum of the > +at_time from the audit_log maybe restrcted for a list of classes. You can leave out restricted for a list of classes as you already detailed this in the Params section. A hash with a single item, C<last_audit_time>, that is the maximum of the at_time from the audit_log.
Attachment #612600 -
Flags: review?(dkl) → review-
Assignee | ||
Comment 14•12 years ago
|
||
I create an new patch but not use the code from comment#13
Attachment #612600 -
Attachment is obsolete: true
Attachment #613552 -
Flags: review?(dkl)
Comment 15•12 years ago
|
||
Comment on attachment 613552 [details] [diff] [review] patch V4 Review of attachment 613552 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine. r=dkl ::: Bugzilla/WebService/Bugzilla.pm @@ +433,5 @@ > + > +=item C<class> (array) - An array of strings representing the class names. > + > +B<Note:> The class names are defined as "Bugzilla::<class_name>". For the product > +use Bugzilla:Product. s/Bugzilla:Product/Bugzilla::Product/ on checkin.
Attachment #613552 -
Flags: review?(dkl) → review+
Updated•12 years ago
|
Flags: approval?
Comment 16•12 years ago
|
||
Comment on attachment 613552 [details] [diff] [review] patch V4 >+ my @class_values_quoted = map {$dbh->quote($_)} @$class_values; >+ if (@class_values_quoted) { >+ $sql_statement .= " WHERE " . $dbh->sql_in('class', \@class_values_quoted); The lack of validation makes me a bit nervous. I think we should check each value passed to $class_values and make sure they are of the form /^Bugzilla(::[a-zA-Z0-9_]+)*$/ to avoid any attempt of SQL injection. This check can be done when calling map {} above.
Updated•12 years ago
|
Assignee: webservice → Frank
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: relnote
Target Milestone: --- → Bugzilla 4.4
Comment 17•12 years ago
|
||
(In reply to Frédéric Buclin from comment #16) > Comment on attachment 613552 [details] [diff] [review] > patch V4 > > >+ my @class_values_quoted = map {$dbh->quote($_)} @$class_values; > >+ if (@class_values_quoted) { > >+ $sql_statement .= " WHERE " . $dbh->sql_in('class', \@class_values_quoted); > > The lack of validation makes me a bit nervous. I think we should check each > value passed to $class_values and make sure they are of the form > > /^Bugzilla(::[a-zA-Z0-9_]+)*$/ > > to avoid any attempt of SQL injection. This check can be done when calling > map {} above. I too would be more comfortable with this additional check for the values. Please attached a revised patch. dkl
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to David Lawrence [:dkl] from comment #17) > (In reply to Frédéric Buclin from comment #16) > > Comment on attachment 613552 [details] [diff] [review] > > patch V4 > > > > >+ my @class_values_quoted = map {$dbh->quote($_)} @$class_values; > > >+ if (@class_values_quoted) { > > >+ $sql_statement .= " WHERE " . $dbh->sql_in('class', \@class_values_quoted); > > > > The lack of validation makes me a bit nervous. I think we should check each > > value passed to $class_values and make sure they are of the form > > > > /^Bugzilla(::[a-zA-Z0-9_]+)*$/ > > > > to avoid any attempt of SQL injection. This check can be done when calling > > map {} above. > > I too would be more comfortable with this additional check for the values. > Please attached a revised patch. > > dkl Here the new patch!
Attachment #613552 -
Attachment is obsolete: true
Attachment #614092 -
Flags: review?(dkl)
Comment 19•12 years ago
|
||
Comment on attachment 614092 [details] [diff] [review] patch V5 Review of attachment 614092 [details] [diff] [review]: ----------------------------------------------------------------- Looks good and works as expected. r=dkl ::: Bugzilla/WebService/Bugzilla.pm @@ +122,5 @@ > + my $sql_statement = "SELECT MAX(at_time) FROM audit_log"; > + my $class_values = $params->{class}; > + my @class_values_quoted; > + foreach my $class_value (@$class_values) { > + push (@class_values_quoted, $dbh->quote($class_value)) if $class_value =~ /^Bugzilla(::[a-zA-Z0-9_]+)*$/; Nit: code lines should be ~80 chars or less. Bring the 'if ...' part down to the next line and indent. Fix on checkin.
Attachment #614092 -
Flags: review?(dkl) → review+
Comment 20•12 years ago
|
||
(In reply to David Lawrence [:dkl] from comment #19) > > + foreach my $class_value (@$class_values) { > > + push (@class_values_quoted, $dbh->quote($class_value)) if $class_value =~ /^Bugzilla(::[a-zA-Z0-9_]+)*$/; Does this code really work? Where are the values detainted? If the DB server throws no error, then I'm opposed to this code as this means we can pass tainted data to the server. dkl's approach in comment 13 where he uses placeholders is the right way to go, with one change: my @class_values = map { trick_taint($_) } @{$param->{class}}; should be my @class_values = map { $_ =~ /^Bugzilla(::[a-zA-Z0-9_]+)*$/ && trick_taint($_) } @{$param->{class}}; as it's not ok to detaint unvalidated data.
Comment 21•12 years ago
|
||
(In reply to Frédéric Buclin from comment #20) > Does this code really work? Where are the values detainted? If the DB server > throws no error, then I'm opposed to this code as this means we can pass > tainted data to the server. dkl's approach in comment 13 where he uses > placeholders is the right way to go, with one change: It does work and by using dbh->quote, each value is being detained. So effectively push (@class_values_quoted, $dbh->quote($class_value)) if $class_value =~ /^Bugzilla(::[a-zA-Z0-9_]+)*$/; is the same as my @class_values = map { $_ =~ /^Bugzilla(::[a-zA-Z0-9_]+)*$/ && trick_taint($_) } @{$param->{class}}; my @class_values_quoted = map { $dbh->quote($_) } @class_values; dkl
Updated•12 years ago
|
Flags: approval? → approval+
Comment 22•12 years ago
|
||
Checked in. Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk modified Bugzilla/WebService/Bugzilla.pm Committed revision 8190
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 23•12 years ago
|
||
(In reply to Frank Becker from comment #10) > The audit_log table has an class field. We can restrict the max only for an > class or a list of classes. [snip] That will break if there are extensions or customizations.
Comment 24•12 years ago
|
||
BTW, you're going to need an index on the class field now. Otherwise your DB servers are going to be in a world of pain when people start to use this API.
Comment 25•12 years ago
|
||
(In reply to Max Kanat-Alexander from comment #24) > BTW, you're going to need an index on the class field now. Otherwise your > DB servers are going to be in a world of pain when people start to use this > API. dkl or Frank: please file a bug for the DB index.
Flags: testcase?
You need to log in
before you can comment on or make changes to this bug.
Description
•