287487 - User with no privs can not add comments to bugs that have a deadline field set.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0

A user that has no privs can not add comments to any bugs that have the deadline
field set.  That's because process_bug.cgi is comparing the deadline form field
with the raw mysql value.  But the two formats are incompatible:

     deadline form field format:   YYYY-MM-DD
     mysql deadline format:        YYYY-MM-DD 00:00:00

Net result, bugzilla assumes the user is trying to change the deadlie field and
aborts with a "illegal_change" user-error message. 



Reproducible: Always

Steps to Reproduce:

Comment 1

[Albert Ting]

Attachment: sample patch

This is a sample patch that updates the FORM value before you call the
CheckCanChangeField function in process_bug.cgi.  It's not perfect, makes
assumption we're only querying a MYSQL database.

IMHO, this is a blocker for 2.20.

Comment 2

[Frédéric Buclin]

Comment on attachment 178433 [details] [diff] [review]
sample patch

This is not the right fix. The conversion to a specific date format has to be
centralized in a separate subroutine. Else this will soon be a mess.

I have a patch ready for that. I will take that bug.

Comment 3

[Frédéric Buclin]

Attachment: centralize date format conversion, v1

Deadlines taken from the DB are converted to the YYYY-MM-DD format using the
new DateConversion() subroutine in order to fit the deadline entered by the
user. The goal of using this subroutine is that we now have a single place
where the date format conversion takes place. This will be easier for
maintenance.

Comment 4

[Frédéric Buclin]

Note that in order to reproduce the problem mentionned by the reporter, you must
be a user who is in the timetracking group but not in the editbugs group.
Moreover, you must not have any privilege related to this bug (neither the
reporter nor the owner/QA contact).

Comment 5

[Max Kanat-Alexander]

Comment on attachment 178474 [details] [diff] [review]
centralize date format conversion, v1

We already have Bugzilla::Util::format_date and sql_date_format.

I suspect that the first one should be used -- all displayed dates are supposed
to be filtered through that function...

Comment 6

[Frédéric Buclin]

Attachment: centralize date format conversion, v2

OK, I reached to use format_time in order to convert deadlines to the
"%Y-%m-%d" format. But this required to rewrite format_time as per bug 288083
which is finally fixed by this patch (I thought it would be more difficult,
which is the reason I opened that bug). The most important point is the
addition of the $format parameter to this function.

I tested all cases where format_time is called and everything works well.

Comment 7

[Frédéric Buclin]

Attachment: centralize date format conversion, v2.1

I forgot to update POD docs.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 178912 [details] [diff] [review]
centralize date format conversion, v2.1

Great. :-) Tested, seems to work. :-)

Comment 9

[Vlad Dascalu]

Comment on attachment 178912 [details] [diff] [review]
centralize date format conversion, v2.1

We have tests nowadays for most of the Utils.pm functions, this one included.
Why not add tests for the format abilities that you've implemented?

Nit: I'd prefer not to remove the given test and cause a regression in
functionality.

That said, the r- is for adding new stuff to Utils.pm without adding simple
tests for the new thing, especially since we have tests for the old behaviour
and it's pretty easy to do that.

Comment 10

[Frédéric Buclin]

Attachment: patch, v3

While adding new tests to 007util.t, I noticed that the timezone should not be
displayed if we are returning a date only. So I modified format_time() a bit:
by default, if $format is not given, the timezone is added; else the timezone
is added only if $format contains %Z or %z.

Comment 11

[Frédéric Buclin]

Note: If you think the last test in 007util.t is useless ("%Y-%m-%d"), I can
remove it on checkin.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 179514 [details] [diff] [review]
patch, v3

>+    if (defined $time) {
>+        $date = time2str($format, $time);
>+        $date .= " " . &::Param('timezone') if $show_timezone;
>+    }
>+    else {
>+        $date = '';
>     }

  That $date = '' is a functionality change from how the subroutine used to
work. It used to just return the value passed in to it, if it couldn't figure
out a valid format.

  Why the change?

>+If no time format is given, it tries to guess it, else it uses a default format.

  And as I mentioned in IRC, this is slightly confusing.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 179514 [details] [diff] [review]
patch, v3

r- until questions are answered.

Comment 14

[Frédéric Buclin]

Attachment: patch, v4

Some (useful ?) comments:

- Util::format_time() is mainly called by templates, using [% my_var FILTER
time %], in which case $format is undefined, and the routine has to "guess" the
right date format used by $dbh->sql_date_format() when defining $my_var.

- Timestamps have all been removed from the DB. So I removed the corresponding
part from format_time() as well as from 007util.t.

- As this routine is used to filter dates, its returned values should be safe!
So I changed its behavior a bit when an invalid string is passed to it.
Previously, it returned this string as is (oops!); now, it returns an empty
string. All values passed to 'FILTER time' come from the DB so they should
never be invalid. But better safe than sorry!

- I updated the POD docs to make one well known reviewer happy. ;)


And yes I tested my patch. Everything works well!

Comment 15

[Max Kanat-Alexander]

Comment on attachment 182903 [details] [diff] [review]
patch, v4

  Yep, this looks correct, and I trust LpSolit's testing of it.

>+the routine has to "guess" the date format passed to $dbh->sql_date_format().

  My only nit (can fix on checkin): "the date format that was passed to"

  Or, alternately, you can remove that implementation note entirely from the
API docs. Implementation details don't need to be in the API docs. You only
need to say something like "If no format is specified, it will use a default
format of YYYY-MM-DD HH:MM:SS TZ"

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Approved for checkin during 2.20 freeze.

Comment 17

[Frédéric Buclin]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.254; previous revision: 1.253
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.26; previous revision: 1.25
done
Checking in t/007util.t;
/cvsroot/mozilla/webtools/bugzilla/t/007util.t,v  <--  007util.t
new revision: 1.6; previous revision: 1.5
done