Tarek Ziadé (:tarek) Reporter
	Description • 14 years ago

if authenticate_user() fails to authenticate the user, and the user does not have an '@', remote_user_original is not defined and the code breaks.

It misses an "else" here: http://hg.mozilla.org/services/server-core/file/98dc4a02cc4c/services/wsgiauth.py#l110

We need an extra test w/ the fix here to cover this combination.


Traceback (most recent call last):
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/services/util.py", line 443, in __call__
    return self.app(environ, start_response)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/lib/python2.6/site-packages/Paste-1.7.5.1-py2.6.egg/paste/translogger.py", line 68, in __call__
    return self.application(environ, replacement_start_response)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/lib/python2.6/site-packages/WebOb-1.0.1-py2.6.egg/webob/dec.py", line 147, in __call__
    resp = self.call_func(req, *args, **self.kwargs)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/lib/python2.6/site-packages/WebOb-1.0.1-py2.6.egg/webob/dec.py", line 208, in call_func
    return self.func(req, *args, **kwargs)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/services/baseapp.py", line 247, in __call__
    self.auth.check(request, match)
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/services/wsgiauth.py", line 66, in check
    match.get('username'))
  File "/home/tarek/dev/hg.mozilla.org/server-full-clean/deps/server-core/services/wsgiauth.py", line 125, in authenticate_user
    if remote_user_original is not None and \
UnboundLocalError: local variable 'remote_user_original' referenced before assignment

	JR Conlin [:jrconlin,:jconlin] Assignee
	Comment 1 • 14 years ago

Actually, removing the prior "if", and always setting remote_user_original will fix the undef (and apparently, there's no way to test for variable definition without trapping it in an exception handler, so correcting that mistake).

I'm a bit concerned about the test, though, since this should have been triggered by the existing bad user exception trap. (userid == None && userid does not contain '@' should have triggered the error) Looking into why that may not have fired.

	Tarek Ziadé (:tarek) Reporter
	Comment 2 • 14 years ago

(In reply to comment #1)
> Actually, removing the prior "if", and always setting remote_user_original will
> fix the undef 

In that case, if remote_user_original is always equal to  user_name, why do we have two variables ?

(and apparently, there's no way to test for variable definition
> without trapping it in an exception handler, so correcting that mistake).
> 
> I'm a bit concerned about the test, though, since this should have been
> triggered by the existing bad user exception trap. (userid == None && userid
> does not contain '@' should have triggered the error) Looking into why that may
> not have fired.

Yeah, we need to make sure our test catch this bug :D

	JR Conlin [:jrconlin,:jconlin] Assignee
	Comment 3 • 14 years ago

> In that case, if remote_user_original is always equal to  user_name, why do we
> have two variables ?

Because user_name is potentially changed and lost at 
http://hg.mozilla.org/services/server-core/file/98dc4a02cc4c/services/wsgiauth.py#l113

(Obviously, the "is None" can be removed from the following test.)

	Tarek Ziadé (:tarek) Reporter
	Comment 4 • 14 years ago

Ok. Turns out hudson is broken in fact, but there's a problem with the email sendings

	JR Conlin [:jrconlin,:jconlin] Assignee
	Comment 5 • 14 years ago

removed '@' conditional.
added test to catch exception.

	JR Conlin [:jrconlin,:jconlin] Assignee
	Comment 6 • 14 years ago

changeset:   572:4a93662d2a88
tag:         tip
user:        JR Conlin <jconlin@mozilla.com>
date:        Tue Feb 08 13:15:07 2011 -0800
summary:     bz 631508: removed conditional and added test for remote_user_original