Last Comment Bug 365697 - email_in.pl is case-sensitive for products due to Bugzilla::User->can_enter_product
: email_in.pl is case-sensitive for products due to Bugzilla::User->can_enter_p...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Incoming Email (show other bugs)
: 2.23.3
: All All
: -- major (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
: 443054 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-02 13:09 PST by Albert Ting
Modified: 2008-07-02 15:46 PDT (History)
2 users (show)
mkanat: approval+
mkanat: approval3.2+
mkanat: approval3.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (661 bytes, patch)
2007-01-03 15:35 PST, Max Kanat-Alexander
LpSolit: review-
Details | Diff | Splinter Review
patch for 3.2 and HEAD, v2 (1.18 KB, patch)
2008-07-01 16:37 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
patch for 3.0.5, v1 (623 bytes, patch)
2008-07-02 15:32 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Albert Ting 2007-01-02 13:09:49 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: 

email_in.pl properly sends a notification when it doesn't recognize a product name.  However, if the product name matches from a case-insensitive point of view, I get a rather obtuse error message:   

    can_enter_product reached an unreachable location.


Reproducible: Always
Comment 1 Max Kanat-Alexander 2007-01-03 14:43:30 PST
Wow, that's really weird. Are you on PostgreSQL or MySQL? Could you tell me what the two products were that you were using, and if they have any group controls on them?
Comment 2 Albert Ting 2007-01-03 14:57:23 PST
I'm using MySQL.  The product names only differed by the case sensitivity, such as  "FireFox" vs. "Firefox".  But only "FireFox" exists, not "Firefox".

I don't use groups right now. 
Comment 3 Max Kanat-Alexander 2007-01-03 15:29:30 PST
Here's the code that's at fault, I'm pretty sure:

    my $can_enter =
        grep($_->name eq $product_name, @{$self->get_enterable_products});
Comment 4 Max Kanat-Alexander 2007-01-03 15:35:25 PST
Created attachment 250394 [details] [diff] [review]
v1

Okay, this should fix it, and it makes sense to me.

Product names are case-insensitive on both MySQL and PostgreSQL.
Comment 5 Frédéric Buclin 2007-01-03 15:54:38 PST
Comment on attachment 250394 [details] [diff] [review]
v1

If we want can_enter_product() to be case-insensitive, then we should also make can_see_product() case-insensitive.

Also, you have to make sure that you don't introduce any regression, neither in enter/post/process_bug.cgi, nor in editproducts/components/milestones/versions.cgi. This makes me think: how could you change the case  of a product with this change, i.e. if you have a product FireFox, and you want to rename it as Firefox? I know we have a bug about that already, and we should think about this first.
Comment 6 Albert Ting 2007-01-08 12:42:40 PST
> This makes me think: how could you change the case  of a product with this 
> change, i.e. if you have a product FireFox, and you want to rename it as 
> Firefox? 

We have the same problem with email addresses, although I can't find a ticket for that. 
Comment 7 Frédéric Buclin 2008-07-01 16:25:22 PDT
*** Bug 443054 has been marked as a duplicate of this bug. ***
Comment 8 Frédéric Buclin 2008-07-01 16:37:50 PDT
Created attachment 327716 [details] [diff] [review]
patch for 3.2 and HEAD, v2

This is IMO cleaner, especially now that we know that lc() has some problems with some locales.
Comment 9 Max Kanat-Alexander 2008-07-01 16:54:56 PDT
Comment on attachment 327716 [details] [diff] [review]
patch for 3.2 and HEAD, v2

Why not just call Product->check?
Comment 10 Frédéric Buclin 2008-07-01 17:16:45 PDT
(In reply to comment #9)
> Why not just call Product->check?

Because ->check will inform the user that the product doesn't exist instead of the current message which says it either doesn't exist or you don't have access to it. This is the kind of information we don't want to leak.
Comment 11 Max Kanat-Alexander 2008-07-02 09:00:47 PDT
Comment on attachment 327716 [details] [diff] [review]
patch for 3.2 and HEAD, v2

Okay, sure, this looks fine then! :-)
Comment 12 Max Kanat-Alexander 2008-07-02 09:01:44 PDT
I'm not sure of the performance impact of that change, so I don't really want to approve it for 3.0.
Comment 13 Frédéric Buclin 2008-07-02 15:32:10 PDT
Created attachment 327872 [details] [diff] [review]
patch for 3.0.5, v1

Minimal change for 3.0.5: I use lc()
Comment 14 Max Kanat-Alexander 2008-07-02 15:33:41 PDT
a3.0=mkanat for the 3.0.5 patch.
Comment 15 Frédéric Buclin 2008-07-02 15:46:40 PDT
tip:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.166; previous revision: 1.165
done

3.1.4:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.164.2.1; previous revision: 1.164
done

3.0.4:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.148.2.7; previous revision: 1.148.2.6
done

Note You need to log in before you can comment on or make changes to this bug.