Webservice for maximum time of audit_log

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
WebService
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Frank Becker, Assigned: Frank Becker)

Tracking

unspecified
Bugzilla 4.4
Bug Flags:
approval +
testcase ?

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 years ago
Created attachment 610634 [details] [diff] [review]
patch V1
Attachment #610634 - Flags: review?(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?
Attachment #610634 - Flags: review?(dkl) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 611147 [details] [diff] [review]
patch V2

rename to last_audit_time as requested in comment#2
Attachment #610634 - Attachment is obsolete: true
Attachment #611147 - Flags: review?(dkl)

Comment 4

6 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 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

6 years ago
Flags: approval?

Comment 6

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

Comment 7

6 years ago
dkl: see comment 4 and comment 6.
(Assignee)

Comment 8

6 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

6 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

6 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?
(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

6 years ago
Created attachment 612600 [details] [diff] [review]
patch V3

new version with parameter class
Attachment #611147 - Attachment is obsolete: true
Attachment #612600 - Flags: review?(dkl)

Updated

6 years ago
Flags: approval?
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

6 years ago
Created attachment 613552 [details] [diff] [review]
patch V4

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

6 years ago
Flags: approval?

Comment 16

6 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

6 years ago
Assignee: webservice → Frank
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: relnote
Target Milestone: --- → Bugzilla 4.4
(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

6 years ago
Created attachment 614092 [details] [diff] [review]
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!
Attachment #613552 - Attachment is obsolete: true
Attachment #614092 - Flags: review?(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.
Attachment #614092 - Flags: review?(dkl) → review+

Comment 20

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

6 years ago
Flags: approval? → approval+
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 23

6 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

6 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

6 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?

Updated

6 years ago
Blocks: 745533

Comment 26

5 years ago
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.