Closed Bug 300403 Opened 19 years ago Closed 19 years ago

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

Categories

(Bugzilla :: Reporting/Charting, defect)

2.20
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: karl, Assigned: karl)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
OS: Linux → All
Version: unspecified → 2.20
Blocks: 297940
Flags: blocking2.20?
Attached patch Patch v1 (obsolete) — Splinter Review
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
(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.
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
Severity: major → critical
Target Milestone: Bugzilla 2.20 → ---
(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
Whiteboard: [patch awaiting review]
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+
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)
Attached patch Patch v1.5Splinter Review
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+
Status: NEW → ASSIGNED
Whiteboard: [patch awaiting review]
Flags: approval?
Flags: approval2.20?
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
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
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: