740536 - Webservice for maximum time of audit_log

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Frank Becker]

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.

Comment 1

[Frank Becker]

Attachment: patch V1

Comment 2

[David Lawrence [:dkl]]

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?

Comment 3

[Frank Becker]

Attachment: patch V2

rename to last_audit_time as requested in comment#2

Comment 4

[Max Kanat-Alexander]

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

[David Lawrence [:dkl]]

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/

Comment 6

[Max Kanat-Alexander]

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.

Comment 7

[Frédéric Buclin]

dkl: see comment 4 and comment 6.

Comment 8

[Frank Becker]

(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

[Max Kanat-Alexander]

(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.

Comment 10

[Frank Becker]

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

[David Lawrence [:dkl]]

(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

Comment 12

[Frank Becker]

Attachment: patch V3

new version with parameter class

Comment 13

[David Lawrence [:dkl]]

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.

Comment 14

[Frank Becker]

Attachment: patch V4

I create an new patch but not use the code from comment#13

Comment 15

[David Lawrence [:dkl]]

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.

Comment 16

[Frédéric Buclin]

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.

Comment 17

[David Lawrence [:dkl]]

(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

Comment 18

[Frank Becker]

Attachment: patch V5

(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!

Comment 19

[David Lawrence [:dkl]]

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.

Comment 20

[Frédéric Buclin]

(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

[David Lawrence [:dkl]]

(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

Comment 22

[David Lawrence [:dkl]]

Checked in.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Bugzilla.pm
Committed revision 8190

Comment 23

[Max Kanat-Alexander]

(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

[Max Kanat-Alexander]

 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

[Frédéric Buclin]

(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.

Comment 26

[Frédéric Buclin]

Added to relnotes for 4.4.