300403 - New Charts errors out, creates new 'add' user, when Env auth method is used

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)

This bug covers the fix to Bugzilla/Auth/Login/WWW/Env.pm as described in bug
297940.

Reproducible: Always

Steps to Reproduce:
1. Configure the chartgroup Param to enable New Charts
2. Set the user_info_class Param to include 'Env' as the first option
3. Log in as a user in the group mentioned in (1)
4. Go into New Charts and add a data set to the list
Actual Results:  
The data set should have been added

Expected Results:  
I get the error "Sorry, you aren't a member of the '*****' group, and so you are
not authorized to use the "New Charts" feature."  It also appears that, on this
page only, I am logged in as user "add".  A new account is created for this
user, although no real name is specified.  This causes Sanity Check to complain
until the account has a valid name/login set or until it's deleted.

Comment 1

[A. Karl Kornel]

Attachment: Patch v1

If the regex causing the problem (line 54) does not match, return undef to
Bugzilla::Auth::Login::WWW so it can try a different method (like CGI).  This
solves the problem for me.

Requesting review from kiko.

Comment 2

[Gervase Markham [:gerv]]

The creation of the new account also sounds like something that shouldn't be
happening, even in an error condition. What causes that?

Gerv

Comment 3

[A. Karl Kornel]

(In reply to comment #2)
> The creation of the new account also sounds like something that shouldn't be
> happening, even in an error condition. What causes that?

It's a combination of line 55, a non-matching regexp on line 54, and lines 60, 78, and 110.  Joel and 
mkanat confirmed that variables such as $1, $2 etc. are not cleared when a regex match fails, so in the 
case of charts.cgi being executed in this way it turns out that $1 is already set from somewhere else 
when the match on line 54 takes place, and $env_email gets this value on line 55.

Since $env_email is now set, the test at line 60 passes.  The test at 78 also passes, since only 
$env_email is defined (not $env_id or $matched_userid).  The fetch from lines 86-95 doesn't return 
anything, so the test at line 96 fails, causing the code at lines 110-123 to be executed, which is where 
the new account is created.

So, by changing $env_email to only get a non-'' value when the regexp matches (which is what the 
attachment should do), the test at line 60 fails and we never get near the code that inserts this new 
user.

Comment 4

[Max Kanat-Alexander]

Setting to "major" only because Env Auth is not used by most installations.

Comment 5

[A. Karl Kornel]

(In reply to comment #4)
> Setting to "major" only because Env Auth is not used by most installations.

For some reason, the target milestone & severity were reset, although
blocking2.20 is still set.  Setting target & severity back to 2.20/major

Comment 6

[A. Karl Kornel]

Comment on attachment 188973 [details] [diff] [review]
Patch v1

Since kiko may not be able to get to this in time, I'm also requesting review
from mkanat.  A review from either one or the other should be fine!

Comment 7

[Marc Schumann [:Wurblzap]]

Comment on attachment 188973 [details] [diff] [review]
Patch v1

Tested; works. Very helpful analysis in comment 3, Karl.

>Index: WWW/Env.pm
>+    if ($env_email =~ /($emailregexp)/) {
>+        $env_email = $1;
>+    } else {
>+        return undef;
>+    }

In an updated patch you may forward my r+ to, please use "uncuddled elses" as
per http://www.bugzilla.org/docs/developer.html#perl, like this:

	[...]
    }
    else {
	[...]

Comment 8

[A. Karl Kornel]

Comment on attachment 188973 [details] [diff] [review]
Patch v1

Yay, we finally have a review!

Cancelling other pending reviews, and marking obsolete in preparation for a new
patch.

Comment 9

[A. Karl Kornel]

Attachment: Patch v1.5

Modification of attachment 188973 [details] [diff] [review], taking into account comment 7.

Review+ from wurblzap carried forward, as granted in comment 7.  Request for
approval immediately follows the submission of this attachment.

Comment 10

[Frédéric Buclin]

tip:

Checking in Bugzilla/Auth/Login/WWW/Env.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/Env.pm,v  <--  Env.pm
new revision: 1.5; previous revision: 1.4
done

2.20rc1:

Checking in Bugzilla/Auth/Login/WWW/Env.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/Env.pm,v  <--  Env.pm
new revision: 1.4.2.1; previous revision: 1.4
done