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

RESOLVED FIXED in Bugzilla 2.20

Status

()

--
major
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: karl, Assigned: karl)

Tracking

2.20
Bugzilla 2.20
Bug Flags:
approval +
approval2.20 +
blocking2.20 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

751 bytes, patch
karl
: review+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
OS: Linux → All
Version: unspecified → 2.20
(Assignee)

Updated

14 years ago
Blocks: 297940
(Assignee)

Updated

14 years ago
Flags: blocking2.20?
(Assignee)

Comment 1

14 years ago
Created attachment 188973 [details] [diff] [review]
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.
Attachment #188973 - Flags: review?(kiko)
The creation of the new account also sounds like something that shouldn't be
happening, even in an error condition. What causes that?

Gerv
(Assignee)

Comment 3

14 years ago
(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

13 years ago
Setting to "major" only because Env Auth is not used by most installations.
Assignee: gerv → karl
Severity: critical → major
Flags: blocking2.20? → blocking2.20+
Target Milestone: --- → Bugzilla 2.20

Updated

13 years ago
Severity: major → critical
Target Milestone: Bugzilla 2.20 → ---
(Assignee)

Comment 5

13 years ago
(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
Severity: critical → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 2.20

Updated

13 years ago
Whiteboard: [patch awaiting review]
(Assignee)

Comment 6

13 years ago
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!
Attachment #188973 - Flags: review?(mkanat)
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 {
	[...]
Attachment #188973 - Flags: review+
(Assignee)

Comment 8

13 years ago
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.
Attachment #188973 - Attachment is obsolete: true
Attachment #188973 - Flags: review?(mkanat)
Attachment #188973 - Flags: review?(kiko)
(Assignee)

Comment 9

13 years ago
Created attachment 190455 [details] [diff] [review]
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.
Attachment #190455 - Flags: review+
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Whiteboard: [patch awaiting review]
(Assignee)

Updated

13 years ago
Flags: approval?
Flags: approval2.20?
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+

Comment 10

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.