From f9631e75a20a4c2904272baeb9497b19085728fc Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Mon, 13 Dec 2010 12:56:46 -0800 Subject: [PATCH] Bug 617477: Fix numerous consistency and behavior issues surrounding Bug.update and Bugzilla::Bug. See https://bugzilla.mozilla.org/show_bug.cgi?id=617477#c2 for details. r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 60 ++++++--- Bugzilla/WebService/Bug.pm | 122 +++++++++++++++--- Bugzilla/WebService/Constants.pm | 19 +++ Bugzilla/WebService/Server.pm | 3 + .../en/default/global/user-error.html.tmpl | 12 +- 5 files changed, 176 insertions(+), 40 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 18a70d0e5f..ceb773aec7 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -1436,12 +1436,14 @@ sub _check_component { sub _check_deadline { my ($invocant, $date) = @_; - - # Check time-tracking permissions. - # deadline() returns '' instead of undef if no deadline is set. - my $current = ref $invocant ? ($invocant->deadline || undef) : undef; - return $current unless Bugzilla->user->is_timetracker; - + + # When filing bugs, we're forgiving and just return undef if + # the user isn't a timetracker. When updating bugs, check_can_change_field + # controls permissions, so we don't want to check them here. + if (!ref $invocant and !Bugzilla->user->is_timetracker) { + return undef; + } + # Validate entered deadline $date = trim($date); return undef if !$date; @@ -1644,8 +1646,7 @@ sub _check_keywords { my %keywords; foreach my $keyword (@$keyword_array) { next unless $keyword; - my $obj = new Bugzilla::Keyword({ name => $keyword }); - ThrowUserError("unknown_keyword", { keyword => $keyword }) if !$obj; + my $obj = Bugzilla::Keyword->check($keyword); $keywords{$obj->id} = $obj; } return [values %keywords]; @@ -1776,6 +1777,10 @@ sub _check_short_desc { if (!defined $short_desc || $short_desc eq '') { ThrowUserError("require_summary"); } + if (length($short_desc) > MAX_FREETEXT_LENGTH) { + ThrowUserError('freetext_too_long', + { field => 'short_desc', text => $short_desc }); + } return $short_desc; } @@ -1868,11 +1873,12 @@ sub _check_target_milestone { sub _check_time { my ($invocant, $time, $field) = @_; - my $current = 0; - if (ref $invocant && $field ne 'work_time') { - $current = $invocant->$field; + # When filing bugs, we're forgiving and just return 0 if + # the user isn't a timetracker. When updating bugs, check_can_change_field + # controls permissions, so we don't want to check them here. + if (!ref $invocant and !Bugzilla->user->is_timetracker) { + return 0; } - return $current unless Bugzilla->user->is_timetracker; $time = trim($time) || 0; ValidateTime($time, $field); @@ -1890,7 +1896,15 @@ sub _check_version { } sub _check_work_time { - return $_[0]->_check_time($_[1], 'work_time'); + my ($self, $input) = @_; + my $time = $self->_check_time($input, 'work_time'); + # Because work_time isn't set by a set_ function, we need to + # do check_can_change_field here manually. + my $privs; + $self->check_can_change_field('work_time', 0, $time, \$privs) + || ThrowUserError('illegal_change', + { field => 'work_time', privs => $privs }); + return $time; } # Custom Field Validators @@ -1947,11 +1961,12 @@ sub _check_datetime_field { sub _check_default_field { return defined $_[1] ? trim($_[1]) : ''; } sub _check_freetext_field { - my ($invocant, $text) = @_; + my ($invocant, $text, $field) = @_; $text = (defined $text) ? trim($text) : ''; if (length($text) > MAX_FREETEXT_LENGTH) { - ThrowUserError('freetext_too_long', { text => $text }); + ThrowUserError('freetext_too_long', + { field => $field, text => $text }); } return $text; } @@ -2228,7 +2243,6 @@ sub reset_assigned_to { sub set_cclist_accessible { $_[0]->set('cclist_accessible', $_[1]); } sub set_comment_is_private { my ($self, $comment_id, $isprivate) = @_; - return unless Bugzilla->user->is_insider; # We also allow people to pass in a hash of comment ids to update. if (ref $comment_id) { @@ -2244,6 +2258,7 @@ sub set_comment_is_private { $isprivate = $isprivate ? 1 : 0; if ($isprivate != $comment->is_private) { + ThrowUserError('user_not_insider') if !Bugzilla->user->is_insider; $self->{comment_isprivate} ||= {}; $self->{comment_isprivate}->{$comment_id} = $isprivate; } @@ -2567,7 +2582,13 @@ sub set_bug_status { else { # We do this here so that we can make sure closed statuses have # resolutions. - my $resolution = delete $params->{resolution} || $self->resolution; + my $resolution = $self->resolution; + # We need to check "defined" to prevent people from passing + # a blank resolution in the WebService, which would otherwise fail + # silently. + if (defined $params->{resolution}) { + $resolution = delete $params->{resolution}; + } $self->set_resolution($resolution, $params); # Changing between closed statuses zeros the remaining time. @@ -3783,7 +3804,8 @@ sub check_can_change_field { } elsif (trim($oldvalue) eq trim($newvalue)) { return 1; # numeric fields need to be compared using == - } elsif (($field eq 'estimated_time' || $field eq 'remaining_time') + } elsif (($field eq 'estimated_time' || $field eq 'remaining_time' + || $field eq 'work_time') && $oldvalue == $newvalue) { return 1; @@ -3818,7 +3840,7 @@ sub check_can_change_field { # $PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED : an empowered user. # Only users in the time-tracking group can change time-tracking fields. - if ( grep($_ eq $field, qw(deadline estimated_time remaining_time)) ) { + if ( grep($_ eq $field, TIMETRACKING_FIELDS) ) { if (!$user->is_timetracker) { $$PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED; return 0; diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index 53e3332556..ffc2e138cd 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -47,7 +47,6 @@ use constant PRODUCT_SPECIFIC_FIELDS => qw(version target_milestone component); use constant DATE_FIELDS => { comments => ['new_since'], search => ['last_change_time', 'creation_time'], - update => ['deadline'], }; use constant BASE64_FIELDS => { @@ -464,7 +463,10 @@ sub update { my $user = Bugzilla->login(LOGIN_REQUIRED); my $dbh = Bugzilla->dbh; - $params = Bugzilla::Bug::map_fields($params, { summary => 1 }); + # We skip certain fields because their set_ methods actually use + # the external names instead of the internal names. + $params = Bugzilla::Bug::map_fields($params, + { summary => 1, platform => 1, severity => 1, url => 1 }); my $ids = delete $params->{ids}; defined $ids || ThrowCodeError('param_required', { param => 'ids' }); @@ -533,6 +535,11 @@ sub update { foreach my $field (keys %changes) { my $change = $changes{$field}; my $api_field = $api_name{$field} || $field; + # We normalize undef to an empty string, so that the API + # stays consistent for things like Deadline that can become + # empty. + $change->[0] = '' if !defined $change->[0]; + $change->[1] = '' if !defined $change->[1]; $hash{changes}->{$api_field} = { removed => $self->type('string', $change->[0]), added => $self->type('string', $change->[1]) @@ -886,7 +893,9 @@ sub _bug_to_hash { if (Bugzilla->user->is_timetracker) { $item{'estimated_time'} = $self->type('double', $bug->estimated_time); $item{'remaining_time'} = $self->type('double', $bug->remaining_time); - $item{'deadline'} = $self->type('dateTime', $bug->deadline); + # No need to format $bug->deadline specially, because Bugzilla::Bug + # already does it for us. + $item{'deadline'} = $self->type('string', $bug->deadline); } if (Bugzilla->user->id) { @@ -1616,7 +1625,8 @@ C The login name of the person who filed this bug (the reporter). =item C -C The day that this bug is due to be completed. +C The day that this bug is due to be completed, in the format +C. If you are not in the time-tracking group, this field will not be included in the return value. @@ -2284,7 +2294,8 @@ A hash with one element, C. This is the id of the newly-filed bug. =item 51 (Invalid Object) -The component you specified is not valid for this Product. +You specified a field value that is invalid. The error message will have +more details. =item 103 (Invalid Alias) @@ -2309,6 +2320,11 @@ you don't have permission to enter bugs in this product. You didn't specify a summary for the bug. +=item 116 (Dependency Loop) + +You specified values in the C or C fields +that would cause a circular dependency between bugs. + =item 504 (Invalid User) Either the QA Contact, Assignee, or CC lists have some invalid user @@ -2331,6 +2347,9 @@ method. Before Bugzilla 4.0, you had to use the undocumented C argument. +=item Error 116 was added in Bugzilla B<4.0>. Before that, dependency +loop errors had a generic code of C<32000>. + =back =back @@ -2502,7 +2521,7 @@ doesn't support aliases or (b) there is no bug with that alias. The id you specified doesn't exist in the database. -=item 108 (Bug Edit Denied) +=item 109 (Bug Edit Denied) You did not have the necessary rights to edit the bug. @@ -2654,9 +2673,8 @@ C The Component the bug is in. =item C -C The Deadline field--a date specifying when the bug must -be completed by. The time specified is ignored--only the date is -significant. +C The Deadline field--a date specifying when the bug must +be completed by, in the format C. =item C @@ -2905,21 +2923,91 @@ Here's an example of what a return value might look like: ] } +Currently, some fields are not tracked in changes: C, +C, and C. This means that they will not +show up in the return value even if they were successfully updated. +This may change in a future version of Bugzilla. + =item B -This function can throw all of the errors that L can throw, plus: +This function can throw all of the errors that L, L, +and L can throw, plus: =over -=item 103 (Invalid Alias) +=item 50 (Empty Field) -Either you tried to set an alias when changing multiple bugs at once, -or the alias you specified is invalid for some reason. +You tried to set some field to be empty, but that field cannot be empty. +The error message will have more details. -=back +=item 52 (Input Not A Number) + +You tried to set a numeric field to a value that wasn't numeric. + +=item 54 (Number Too Large) + +You tried to set a numeric field to a value larger than that field can +accept. + +=item 55 (Number Too Small) + +You tried to set a negative value in a numeric field that does not accept +negative values. + +=item 56 (Bad Date/Time) + +You specified an invalid date or time in a date/time field (such as +the C field or a custom date/time field). + +=item 112 (See Also Invalid) + +You attempted to add an invalid value to the C field. + +=item 115 (Permission Denied) + +You don't have permission to change a particular field to a particular value. +The error message will have more detail. -FIXME: Plus a whole load of other errors that we haven't documented yet, -which we won't even know about until after we do QA for 4.0. +=item 116 (Dependency Loop) + +You specified a value in the C or C fields that causes +a dependency loop. + +=item 117 (Invalid Comment ID) + +You specified a comment id in C that isn't on this bug. + +=item 118 (Duplicate Loop) + +You specified a value for C that causes an infinite loop of +duplicates. + +=item 119 (dupe_of Required) + +You changed the resolution to C but did not specify a value +for the C field. + +=item 120 (Group Add/Remove Denied) + +You tried to add or remove a group that you don't have permission to modify +for this bug, or you tried to add a group that isn't valid in this product. + +=item 121 (Resolution Required) + +You tried to set the C field to a closed status, but you didn't +specify a resolution. + +=item 122 (Resolution On Open Status) + +This bug has an open status, but you specified a value for the C +field. + +=item 123 (Invalid Status Transition) + +You tried to change from one status to another, but the status workflow +rules don't allow that change. + +=back =item B @@ -3017,7 +3105,7 @@ This method can throw all of the errors that L throws, plus: =over -=item 108 (Bug Edit Denied) +=item 109 (Bug Edit Denied) You did not have the necessary rights to edit the bug. diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm index 383f148f53..8fbc7c0979 100644 --- a/Bugzilla/WebService/Constants.pm +++ b/Bugzilla/WebService/Constants.pm @@ -49,13 +49,17 @@ our @EXPORT = qw( use constant WS_ERROR_CODE => { # Generic errors (Bugzilla::Object and others) are 50-99. object_not_specified => 50, + reassign_to_empty => 50, param_required => 50, params_required => 50, + undefined_field => 50, object_does_not_exist => 51, param_must_be_numeric => 52, number_not_numeric => 52, param_invalid => 53, number_too_large => 54, + number_too_small => 55, + illegal_date => 56, # Bug errors usually occupy the 100-200 range. improper_bug_id_field_value => 100, bug_id_does_not_exist => 101, @@ -88,6 +92,7 @@ use constant WS_ERROR_CODE => { comment_is_private => 110, comment_id_invalid => 111, comment_too_long => 114, + comment_invalid_isprivate => 117, # See Also errors bug_url_invalid => 112, bug_url_too_long => 112, @@ -96,6 +101,20 @@ use constant WS_ERROR_CODE => { # Note: 114 is above in the Comment-related section. # Bug update errors illegal_change => 115, + # Dependency errors + dependency_loop_single => 116, + dependency_loop_multi => 116, + # Note: 117 is above in the Comment-related section. + # Dup errors + dupe_loop_detected => 118, + dupe_id_required => 119, + # Group errors + group_change_denied => 120, + group_invalid_restriction => 120, + # Status/Resolution errors + missing_resolution => 121, + resolution_not_allowed => 122, + illegal_bug_status_transition => 123, # Authentication errors are usually 300-400. invalid_username_or_password => 300, diff --git a/Bugzilla/WebService/Server.pm b/Bugzilla/WebService/Server.pm index f4b3600d4e..4e03152196 100644 --- a/Bugzilla/WebService/Server.pm +++ b/Bugzilla/WebService/Server.pm @@ -36,6 +36,9 @@ sub datetime_format_inbound { my ($self, $time) = @_; my $converted = datetime_from($time, Bugzilla->local_timezone); + if (!defined $converted) { + ThrowUserError('illegal_date', { date => $time }); + } $time = $converted->ymd() . ' ' . $converted->hms(); return $time } diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 83f51b5553..bd9fb0a5b1 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -693,10 +693,10 @@ [% ELSIF error == "freetext_too_long" %] [% title = "Text Too Long" %] - The text you entered is too long ([% text.length FILTER html %] characters, - above the maximum length allowed of [% constants.MAX_FREETEXT_LENGTH FILTER none %] - characters): -

[% text FILTER html %]

+ The text you entered in the [% field_descs.$field FILTER html %] + field is too long ([% text.length FILTER html %] characters, + above the maximum length allowed of + [%+ constants.MAX_FREETEXT_LENGTH FILTER none %] characters). [% ELSIF error == "group_cannot_delete" %] [% title = "Cannot Delete Group" %] @@ -1298,6 +1298,10 @@ Either you mis-typed the name or that user has not yet registered for a [% terms.Bugzilla %] account. [% END %] + [% IF class == "Bugzilla::Keyword" %] + The legal keyword names are listed + here. + [% END %] [% ELSIF error == "old_password_incorrect" %] [% title = "Incorrect Old Password" %] -- 2.47.2