631508 - authenticate_user broken with 'remote_user_original'

Status: RESOLVED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Tarek Ziadé (:tarek)]

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

Comment 1

[JR Conlin [:jrconlin,:jconlin]]

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.

Comment 2

[Tarek Ziadé (:tarek)]

(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

Comment 3

[JR Conlin [:jrconlin,:jconlin]]

> 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.)

Comment 4

[Tarek Ziadé (:tarek)]

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

Comment 5

[JR Conlin [:jrconlin,:jconlin]]

Attachment: removed conditional

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

Comment 6

[JR Conlin [:jrconlin,:jconlin]]

changeset:   572:4a93662d2a88
tag:         tip
user:        JR Conlin <[e-mail]>
date:        Tue Feb 08 13:15:07 2011 -0800
summary:     bz 631508: removed conditional and added test for remote_user_original