300410 - Bugzilla::Auth needs to be restructured to not require a BEGIN block

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P2)

Description

[Max Kanat-Alexander]

Right now Bugzilla::Auth does some very funky stuff inside of a BEGIN block,
where it "becomes" another module based on a parameter.

Under mod_perl, this won't work. BEGIN blocks run only once for the lifetime of
an Apache process, so Bugzilla would never pick up a change in the auth-method
params.

Comment 1

[Max Kanat-Alexander]

I've confirmed on landfill that Bugzilla::Auth, in its current state, fails
miserably under mod_perl:

Can't locate object method "authenticate" via package "Bugzilla::Auth" at
/var/www/html/mod_perl/Bugzilla/Auth/Login/WWW/CGI.pm line 57.

Comment 2

[Max Kanat-Alexander]

Attachment: Re-write Bugzilla::Auth (Work In Progress)

This is a nearly-complete re-write of the Bugzilla::Auth structure. I'll explain it later, but basically, there's an "info getter," a "login verifier," and a "login persister." It uses all the same old code from the old Bugzilla::Auth, for the most part, just moved around a lot.

This is defintely not complete; it needs some re-architecture to get LDAP auth to work properly again.

Comment 3

[Max Kanat-Alexander]

Attachment: v1

Here is, basically, a complete re-write of the Auth system. I kept almost all of the old Auth code, I just moved it around a *lot*.

Note that in addition to what you see in this patch, the following files/directories were completely removed:

Bugzilla/Auth/Login/WWW
Bugzila/Auth/README
template/en/default/account/auth/ldap-error.html.tmpl

Explanation of New Auth System
------------------------------

The basic idea is that you get an instance of Bugzilla::Auth, and you ask it to return you a valid user.

Here's what Bugzilla::Auth does internally when you call Bugzilla::Auth->login():

1) Get info about the user. This could either be from the Environment 
   (Bugzilla::Auth::Login::Env) or from the passed-in CGI variables 
   (Bugzilla::Auth::Login::CGI).

  a) While we're getting info about the user, we need to determine whether
     or not we've been passed a valid user name. We check this by checking
     the current "account source." At this point, a user account could come
     from the database, or it could come from LDAP.

     Note that certain Login methods (such as Env) actually insert a new
     user into the database here, if the user isn't already in the
     database.

     Certain account sources (such as LDAP) also are capable of inserting
     a new user. For example, "mkanat" is not already in the database,
     but is a valid LDAP user. So, we insert him into the database.

     That may sound slightly confusing. If it does, just ignore it for now.
     You may start to understand when you read the code.

At the end of Step 1, the "Info Getter" (an instance of Bugzilla::Auth::Login, from above) returns a valid Bugzilla::User object, or it returns a failure with a failure code.

Note: This means that now, LDAP accounts are created as soon as we determine that they are valid accounts, not as soon as we verify the password. I understand that some people may be uncomfortable with this, but I think it's okay, as it will allow us to do things in the future like add LDAP support to User::match.

3) We check the valid user's password against our "Password Verifier." 
   (Bugzilla::Auth::Verify). Currently that could be the database, or
   it could be LDAP. If we succeed, we return a valid Bugzilla::User
   object. If we fail, we return a failure with a failure code.

4) If we were successful:
     We make the user's login persist, so that he doesn't have to log in
     on every page. We do this with our "Persister." For now we only
     have one Persister, Bugzilla::Auth::Persist::Cookie. That is,
     to be clearer, at this point we send the user a cookie.

     We also return a valid user object.

   If we failed:
     We handle the failure, in _handle_verify_result.

How To Review This Patch
------------------------

This is a huge patch, so I thought I'd give some tips on where to start and where to go, in order to understand things.

1) Look at Bugzilla->login. That's where everything starts.

2) Look at Bugzilla::Auth->login. (You can also look at Bugzilla::Auth->new, 
   if you want here, too.) This calls a lot of subroutines, but you don't
   have to look at them for now. They are explained, above.

3) Look at Bugzilla::Auth->_handle_verify_result.

4) Look at Bugzilla::Auth::Login and all its sub-modules (do Stack last).
5) Look at Bugzilla::Auth::AccountSource and all its sub-modules (do Stack 
   last).
6) Look at Bugzilla::Auth::Verify and all its sub-modules (do Stack last).
7) Look at Bugzilla::Auth::Persist::Cookie
8) Look at the various code changes that I had to make to other files in 
   Bugzilla.

A Note About POD
----------------

I removed all the POD for Bugzilla::Auth, without adding any new POD. Don't worry, I plan to add all the POD back, just not in this first patch.

Comment 4

[Max Kanat-Alexander]

A few other notes:

1) LDAP auth as it is in the above patch hasn't actually been tested, but I know that it compiles properly.

2) In doing the reviews, it's probably just simplest to look at the "basic" modules first: Login::CGI, AccountSource::DB, and Verify::DB.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 214162 [details] [diff] [review]
v1

Okay, I'm slightly re-thinking this architecture.

I don't want to create user accounts without verifying their passwords, when logging in, but I want to have the *option* of creating user accounts without verifying the password. I think I can do this.

Comment 6

[Max Kanat-Alexander]

Attachment: v2

Okay, this is a new version. Here's how this one works:

1) We get the user data from a Cookie, the CGI, or the Env module.
2) We pass that user data on to check if it has a valid username.
3) We pass *that* user data on to check if the password is valid.
4) If necessary, we create the account or update any of its details.

If any of those steps fail, we have a handler called _handle_login_result that deals with it.

Much simpler, yes? :-)

This keeps all of the functionality of our current Auth, and adds more flexibility. You'll notice that "AccountSource" is gone from this version; it was unnecessary. You can still have your usernames come from a different place than your passwords (which is useful for things like PAM, for example), but both of those verification steps are handled by Bugzilla::Auth::Verify.

This version doesn't yet have PAM in it, and it also is mising a few error messages in code-error.html.tmpl. Other than that, it works very nicely, as tested on landfill.

The information about "How to Review This Patch" still stands.

Comment 7

[Frédéric Buclin]

Comment on attachment 214298 [details] [diff] [review]
v2

this is a huge patch, and I never really investigated the auth code... so I need some help to make sure everything runs correctly.

Comment 8

[Frédéric Buclin]

mkanat, you said you will post a new patch with POD in it, right? So, I will wait for it before starting reviewing it.

Comment 9

[Frédéric Buclin]

Comment on attachment 214298 [details] [diff] [review]
v2

>Index: Bugzilla.pm

> sub login {
>+    # Don't want to log in twice.
>+    return if Bugzilla->user->id;

We usually write: my $user = Bugzilla->user(); to get the current user object. Now an undefined value is returned, which will break most CGIs.


>+    if (($_cgi && Bugzilla->cgi->param('GoAheadAndLogIn')) 
>+        || Param('requirelogin')) 
>+    {
>+         $type = LOGIN_REQUIRED;
>+    } 

Now I cannot create a new user account anymore, nor access index.cgi when I'm logged out and 'requirelogin' is on.


>+    Bugzilla::Auth::Persist::Cookie->logout($_user, $option);
>+    Bugzilla->logout_request();

Nit: Having Bugzilla::Auth->logout() would be better IMO.



>Index: token.cgi

>+    unless (Bugzilla->user->can_change_password) {

You probably mean Bugzilla->user->authorizer->can_change_password.



>Index: Bugzilla/Auth.pm

>+sub new {
>+    $params->{Verify}  ||= Param('user_verify_class');
>+    $params->{Account} ||= $params->{Verify};
>+
>+    $self->{_account_source} = 
>+        new Bugzilla::Auth::Verify::Stack($params->{Account});
>+    $self->{_verifier} = new Bugzilla::Auth::Verify::Stack($params->{Verify});

Nit: as $params->{Account} = $params->{Verify} by default, I'm not sure we need to distinguish them for now. But that's probably OK.


>+sub login {
>+    my ($self, $type) = @_;

$type appears in many places in this routine, but I cannot find a place either here or in one of the called methods where it's used. I think $type is for Bugzilla::CGI only.


>+        # See if that's a valid user account
>+        $login_info = $self->{_account_source}->check_username($login_info);

>+        # And see if the password is valid.
>+        $login_info = $self->{_verifier}->check_password($login_info, $type);

I still don't understand why you check the username here as check_password() will do this check again. IMO, Auth::login() should call check_credentials() or something like that, all both the username and password should be checked internally in Verify/Foo.pm.


>+    $login_info = 
>+        $self->{_account_source}->create_or_update_user($login_info, $type);
>+
>+    # Make sure the user isn't disabled.
>+    my $user = $login_info->{user};
>+    if ($user->disabledtext) {
>+        return $self->_handle_login_result({ failure => AUTH_DISABLED,
>+                                              user    => $user }, $type);
>+    }


Isn't it too late to check whether the user account is disabled or not? Because create_or_update_user() has already been called at this point and this method permits to change data in the DB, which is bad IMO.


>Index: Bugzilla/Constants.pm

>+    AUTH_NO_SUCH_USER

Nit: my opinion is that we don't want tho distinguish invalid usernames from incorrect passwords.



>Index: template/en/default/index.html.tmpl

>+  [% IF user.authorizer.user_can_create_account && Param('createemailregexp') %]

Nit: IMO, it makes more sense to first check whether 'createemailregexp' is on. This comment applies in all templates.


>Index: template/en/default/account/auth/login-small.html.tmpl

>-      [% IF has_db != 0 %]
>+      [% IF can_change_password %]

Where is this variable coming from?


>+++ Bugzilla/Auth/Verify.pm	2006-03-07 03:51:10.000000000 -0800

>+sub can_change_password {
>+    return $_[0]->can('change_password');
>+}

What is can()???? Probably can_change_password(). At the least, the UI in the footer and in index.cgi is incorrect.



>+++ Bugzilla/Auth/Login/Cookie.pm	2006-03-07 03:09:15.000000000 -0800

>+sub get_login_info {

>+        trick_taint($user_id);

Nit: detaint_natural() would make more sense IMO.


>+            my $username = login_to_id($user_id);

Huh? login_to_id() requires a login name as a parameter, not a user ID.


>+    } else {
>+        $login_result = { failure => AUTH_NODATA };
>+    }

Nit: maybe should you return earlier if there is missing data, to avoid this big IF block.


Your updated patch should remove Auth/Login/WWW* completely.

I didn't look at all files in great details (I already spent many hours on it), but it globally looks good.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> > sub login {
> >+    # Don't want to log in twice.
> >+    return if Bugzilla->user->id;
> 
> We usually write: my $user = Bugzilla->user(); to get the current user object.
> Now an undefined value is returned, which will break most CGIs.


Err... I meant we usually write: my $user = Bugzilla->login().

Comment 11

[Max Kanat-Alexander]

Attachment: v3

Okay, I fixed everything from your comments that needed fixing. I also added a lot of POD, so you can read that and it should make things make a lot more sense.

I don't have time to actually test this, but it passes runtests. (Well, except for throwables, which needs to understand AUTH_ERROR return codes.)

Comment 12

[Frédéric Buclin]

Note to self:

<mkanat> Oh, and get_persisted_user should have been deleted, and I forgot to do that.

Comment 13

[Frédéric Buclin]

*** ERROR: =item without previous =over at line 314 in file Bugzilla/Auth.pm
*** WARNING: No items in =over (at line 316) / =back list at line 325 in file Bugzilla/Auth.pm
*** ERROR: =over on line 382 without closing =back (at head1) at line 391 in file Bugzilla/Auth.pm
not ok 59 - Bugzilla/Auth.pm has incorrect POD syntax --ERROR

Comment 14

[Frédéric Buclin]

When trying to impersonate a user, I get the following error:

Can't locate object method "get_flag" via package "Bugzilla::User" at /var/www/html/bugzilla-pg/relogin.cgi line 95.

Comment 15

[Frédéric Buclin]

Comment on attachment 218743 [details] [diff] [review]
v3

>Index: userprefs.cgi

When changing my password, the footer is then the same as if I were logged out. That's confusing! Looks like $user->authorizer->can_logout() works incorrectly (or something else, I don't know).



>Index: Bugzilla/User.pm

>-sub get_flag {

relogin.cgi and some templates still use $user.get_flag(), and the POD of User.pm still mention get_flag and set_flags. Impersonating a user crashes Bugzilla.



>Index: Bugzilla/Auth/Verify/LDAP.pm

>+sub check_username {

Are you sure you don't have to check that a username and a password are given here? AFAIK, a user ID could be returned by get_login_info(), which is valid per the POD, but LDAP requires a username, not a user ID. This check existed initially.


>+sub create_or_update_user {

>+    # If bz_username equals username, then there's a chance we need
>+    # to re-check the LDAP username. However, if we have a user_id
>+    # already, we don't need to check any of that info.

I don't understand this comment. Could you explain why you have to re-check something?


>+        # If possible, we try to get this information by binding with the
>+        # username and password. If that's not possible, we just use an
>+        # anonymous bind.
>+        my $password_check = $self->ldap->check_password($params);
>+        if ($password_check->{failure}) {
>+            $self->_bind_ldap_anonymously();
>         }

What are we doing here? And why is an anonymous connection fine?


>+        $params = self->_set_user_details($params, 'require email');

_set_user_details() doesn't exist.


>+sub _bz_search_params {
>+    my ($username) = @_;
>+    return (base   => Param("LDAPBaseDN"),
>+            scope  => "sub",
>+            filter => '(&(' . Param("LDAPuidattribute") . "=$username)"
>+                      . Param("LDAPfilter") . ')');
>+}

You removed "attrs => ['dn']" from one of the queries; is that intentional? What you did looks correct, but I want a confirmation.


>+sub _bind_ldap_anonymously {

>+    ThrowCodeError("ldap_connect_failed", {errstr => $bind_result->error})
>+        if $bind_result->code;

This error expects a 'server' parameter, not 'errstr'. You probably need another error message for this one as "ldap_connect_failed" is already in use elsewhere.


>+sub ldap {

>+    ThrowCodeError("ldap_server_not_defined") unless $server;

Now that you are moving all errors into global/code-error.html.tmpl, you can remove account/auth/ldap-error.html.tmpl completely.


>+    my $conn_string = "$protocol://$server:$port";
>+    $self->{ldap} = new Net::LDAP($conn_string) 

Nit: the doc says that by default version 3 is used. But the original code wrote it explicitly. Unless there are old versions of Net::LDAP using an older version, that's probably OK not to mention it.


So you never explicitly call $self->{ldap}->unbind(); is that intentional?



>Index: template/en/default/account/auth/login.html.tmpl

The INTERFACE must be updated. Auth::has_db no longer exists.



>Index: template/en/default/global/code-error.html.tmpl

>-  [% ELSIF error == "auth_err" %]
>-    [% title = "Internal Authentication Error" %]
>-    [% INCLUDE "account/auth/$authmethod-error.html.tmpl" %]

See, account/auth/ldap-error.html.tmpl is now useless (see my comment above). :)



>+++ Bugzilla/Auth/Verify.pm	2006-04-17 16:17:22.000000000 -0700

>+sub create_or_update_user {

>+            return { failure => AUTH_ERROR, error => "extern_id_conflict",
>+                     details => {extern_id   => $extern_id,
>+                                 extern_user => $extern_name,
>+                                 username    => $username} };

You inverted 'extern_user' and 'username', either here or in the error message.


>+              || return { failure => AUTH_ERROR, 
>+                          error   => 'auth_illegal_username',
>+                          details => {username => $username} };

'auth_illegal_username' is not defined in code-error.html.tmpl.


>+        if ($extern_id && $username_user_id && !$extern_user_id) {
>+            $dbh->do('UPDATE profiles SET extern_id = ? WHERE userid = ?',
>+                     undef, $extern_id, $username_user_id);
>+            $extern_user_id = $extern_id;
>+        }

$extern_user_id must be an integer, but $extern_id is a string. This last line is wrong (and should probably be deleted).


>+        # Finally, at this point, one of these will give us a valid user id.
>+        $user_id = $extern_user_id || $username_user_id;

Unless I'm wrong, $user_id = $username_user_id.


>+    ThrowCodeError('bad_arg', {argument => 'params', function =>
>+        'Bugzilla::Auth::AccountSource::create_or_update_user'})

The name of the method is 'Bugzilla::Auth::Verify::create_or_update_user'.



>+++ Bugzilla/Auth/Login/Env.pm	2006-03-07 03:12:35.000000000 -0800

>+sub get_login_info {

>+    my $env_id       = $ENV{Param("auth_env_id")} || '';
>+    my $env_email    = $ENV{Param("auth_env_email")} || '';
>+    my $env_realname = $ENV{Param("auth_env_realname")} || '';
>+
>+    return { failure => AUTH_NODATA } if !$env_email;

In the original code, $env_email was checked against Param("emailregexp") which is no longer true here. Probably should you bring back this regexp.



>+++ Bugzilla/Auth/Verify/Stack.pm	2006-03-07 01:57:58.000000000 -0800

>+sub check_password {

>+    foreach my $object (@{$self->{_stack}}) {
>+        $result = $object->check_password(@_);

I'm definitely not convinced by this way of doing things. IMO, you should keep somewhere the information about the successful validation method and use it instead of looping over all possible validations till you find one which suits what you want to do. I'm talking here about $self->{_account_source} versus $self->{_verifier}. They should be a single object IMO. If you definitely want to distinguish them, please add a comment exlaining why.



>Index: Bugzilla.pm

>+# CGIs that are exempt from the 'requirelogin' parameter.
>+use constant REQUIRELOGIN_EXEMPT => [
>+    'index.cgi',
>+    'createaccount.cgi'
>+];
>
> sub login {

>+    if (( $_cgi && Bugzilla->cgi->param('GoAheadAndLogIn') ) 
>+          || ( Param('requirelogin')
>+               && lsearch(REQUIRELOGIN_EXEMPT, basename($0)) == -1 ))
>+    {
>+         $type = LOGIN_REQUIRED;
>+    } 
>+    elsif (!$type) {
>+        $type = LOGIN_OPTIONAL;
>+    }

We don't want all this code, really. Bugzilla.pm is not the right place to decide what should be mandatory and what shouldn't. The following code should work:

    $type = LOGIN_REQUIRED if Bugzilla->cgi->param('GoAheadAndLogIn');
    if (!$type || $type == LOGIN_NORMAL) {
        $type = Param('requirelogin') ? LOGIN_REQUIRED : LOGIN_OPTIONAL;
    }

Both index.cgi and createaccount.cgi contains "Bugzilla->login(LOGIN_OPTIONAL);" so they will successfully go through this check.



>+++ Bugzilla/Auth/Persist/Cookie.pm	2006-03-05 02:48:57.000000000 -0800

>+sub logout {
>+    my ($self, $user, $type) = @_;

Note that logout() requires a Bugzilla::User object as argument, but Auth.pm doesn't give any when logging out the user because the user account is disabled.



>Index: Bugzilla/Auth.pm

>+    $self->{_account_source} = 
>+        new Bugzilla::Auth::Verify::Stack($params->{Account});
>+    $self->{_verifier} = new Bugzilla::Auth::Verify::Stack($params->{Verify});

As said above, you should really add a comment explaining why you distinguish both.


>+sub login {

>+        # See if that's a valid user account
>+        $login_info = $self->{_account_source}->check_username($login_info);

>+        # And see if the password is valid.
>+        $login_info = $self->{_verifier}->check_password($login_info, $type);

I still think you could bypass some checks this way, using one method to validate a username, using another one to validate the password, but finally that you haven't the username-password binomial anywhere.

Moreover, please explain why you validate the username here as check_password() will do this check again? And maybe the validation check_password() will do will be different from the one check_username() did for the reason I gave above. This sounds wrong to me, really.

Minor point: when does check_password() use $type? Probably you should remove it.


>+sub _handle_login_result {

>+            case AUTH_DISABLED {
>+                $self->{_persister}->logout(LOGOUT_ALL);

As said above, logout() expects to get the user object to log out as argument.


>+=head2 Login
>+
>+=item C<login($type)>
>+
>+=over 4

./runtests.pl -v 11 fails here and in some other places of Auth.pm too, see comment 13.



And finally, you should either update or delete Bugzilla/Auth/README which is obsolete now.


I haven't done heavy tests yet, especially because I cannot use impersonation. ;)

Comment 16

[Max Kanat-Alexander]

Attachment: v4

Okay. Haven't tested this patch, but it passes runtests.

(In reply to comment #15)
> When changing my password, the footer is then the same as if I were logged 
> out.
> That's confusing! Looks like $user->authorizer->can_logout() works incorrectly
> (or something else, I don't know).

  Ahh. Was a bug in Bugzilla->logout. Fixed.

> relogin.cgi and some templates still use $user.get_flag(), and the POD of
> User.pm still mention get_flag and set_flags. Impersonating a user crashes
> Bugzilla.

  Fixed.

> Are you sure you don't have to check that a username and a password are given
> here? AFAIK, a user ID could be returned by get_login_info(), which is valid
> per the POD, but LDAP requires a username, not a user ID. This check existed
> initially.

  Hrm. I clarified the POD in the STRUCTURE section under "Info Getter."

> >+    # If bz_username equals username, then there's a chance we need
> >+    # to re-check the LDAP username. However, if we have a user_id
> >+    # already, we don't need to check any of that info.
> 
> I don't understand this comment. Could you explain why you have to re-check
> something?

  Yeah. I actually forgot myself, but I put in the explanation that I think is true, in a comment there now.

> >+        # If possible, we try to get this information by binding with the
> >+        # username and password. If that's not possible, we just use an
> >+        # anonymous bind.
> >+        my $password_check = $self->ldap->check_password($params);
> >+        if ($password_check->{failure}) {
> >+            $self->_bind_ldap_anonymously();
> >         }
> 
> What are we doing here? And why is an anonymous connection fine?

  We're about to try to get the realname and the username from LDAP.

  I clarified the comment.

> >+        $params = self->_set_user_details($params, 'require email');
> 
> _set_user_details() doesn't exist.

  Yeah, fixed that. :-) As you can tell, I haven't tested the LDAP part of this code at ALL.

> >+sub _bz_search_params {
> >+    my ($username) = @_;
> >+    return (base   => Param("LDAPBaseDN"),
> >+            scope  => "sub",
> >+            filter => '(&(' . Param("LDAPuidattribute") . "=$username)"
> >+                      . Param("LDAPfilter") . ')');
> >+}
> 
> You removed "attrs => ['dn']" from one of the queries; is that intentional?
> What you did looks correct, but I want a confirmation.

  Yeah, it was intentional. I want all the attrs, not just that one.

> >+sub _bind_ldap_anonymously {
> 
> >+    ThrowCodeError("ldap_connect_failed", {errstr => $bind_result->error})
> >+        if $bind_result->code;
> 
> This error expects a 'server' parameter, not 'errstr'. You probably need
> another error message for this one as "ldap_connect_failed" is already in use
> elsewhere.

  Ahh, you're right. Fixed.

> Now that you are moving all errors into global/code-error.html.tmpl, you can
> remove account/auth/ldap-error.html.tmpl completely.

  Ahh, right. Thanks for the reminder.

> Nit: the doc says that by default version 3 is used. But the original code
> wrote it explicitly. Unless there are old versions of Net::LDAP using an older
> version, that's probably OK not to mention it.

  Yeah, I think it's fine. I didn't check the CPAN docs, but I think we're okay.

> So you never explicitly call $self->{ldap}->unbind(); is that intentional?

  Yeah, it unbinds automatically when the object is destroyed.

> >Index: template/en/default/account/auth/login.html.tmpl
> 
> The INTERFACE must be updated. Auth::has_db no longer exists.

  Fixed.

> >Index: template/en/default/global/code-error.html.tmpl
> 

> >+            return { failure => AUTH_ERROR, error => "extern_id_conflict",
> >+                     details => {extern_id   => $extern_id,
> >+                                 extern_user => $extern_name,
> >+                                 username    => $username} };
> 
> You inverted 'extern_user' and 'username', either here or in the error 
> message.

  It looks fine to me, in the code. What are you seeing?

> 'auth_illegal_username' is not defined in code-error.html.tmpl.

  You're right. Should have been illegal_email_address. Fixed.

> >+        if ($extern_id && $username_user_id && !$extern_user_id) {
> >+            $dbh->do('UPDATE profiles SET extern_id = ? WHERE userid = ?',
> >+                     undef, $extern_id, $username_user_id);
> >+            $extern_user_id = $extern_id;
> >+        }
> 
> $extern_user_id must be an integer, but $extern_id is a string. This last line
> is wrong (and should probably be deleted).

  You're right. Fixed it.

> >+        # Finally, at this point, one of these will give us a valid user id.
> >+        $user_id = $extern_user_id || $username_user_id;
> 
> Unless I'm wrong, $user_id = $username_user_id.

  No, sometimes $extern_user_id is actually set and $username_user_id isn't. Like, when we're creating somebody through Env, I think.

> The name of the method is 'Bugzilla::Auth::Verify::create_or_update_user'.

  Fixed.

> In the original code, $env_email was checked against Param("emailregexp") 
> which is no longer true here. Probably should you bring back this regexp.

  create_or_update_account handles that, now.

> If you definitely want
> to distinguish them, please add a comment exlaining why.

  Does the POD not explain why? It's so that I can check the password somewhere other than where I check the username.

> We don't want all this code, really. Bugzilla.pm is not the right place to
> decide what should be mandatory and what shouldn't. The following code should
> work:

  You're right. Fixed.

> >+sub logout {
> >+    my ($self, $user, $type) = @_;
> 
> Note that logout() requires a Bugzilla::User object as argument, but Auth.pm
> doesn't give any when logging out the user because the user account is
> disabled.

  Okay. I fixed that and I re-worked the way that Persist::logout() is called.

> I still think you could bypass some checks this way, using one method to
> validate a username, using another one to validate the password, but finally
> that you haven't the username-password binomial anywhere.

  How can I check a password without a username? Impossible. I can check a username without a password, but I can't check a password without a username.

> Minor point: when does check_password() use $type? Probably you should remove
> it.

  Right. Fixed.

> And finally, you should either update or delete Bugzilla/Auth/README which is
> obsolete now.

  Thanks for the reminder.

Comment 17

[Frédéric Buclin]

Comment on attachment 221491 [details] [diff] [review]
v4

I didn't look at the code, only did some tests:

- Verify::create_or_update() may return the 'illegal_email_address' error tag which will be passed to ThrowCodeError(), but this error tag is defined in user-error.html.tmpl.

- relogin.cgi: I can impersonate a user even when entering the wrong password in the sudo-prepare screen. Looks like parameters are either incorrectly passed to Bugzilla->login(), or Bugzilla->login() doesn't care about this new password as it already has a user being logged in (we ask the password again when impersonating someone else to make sure the user is really the one we expect to be).

- When the 'requirelogin' parameter is turned on, I cannot access index.cgi and createaccount.cgi anymore. The login form appears in all cases.

- Bugzilla::Auth::Verify at line 96 complains that ThrowCodeError() is unknown. When adding 'use Bugzilla::Error;' to this file, I now get the following error when using the ENV method:
"Bad argument params sent to Bugzilla::Auth::Verify::create_or_update_user function."
I guess we should never come here.

Comment 18

[Max Kanat-Alexander]

Attachment: v5

Okay, I fixed all of your comments.

Comment 19

[Frédéric Buclin]

Comment on attachment 221592 [details] [diff] [review]
v5

Again tests only, no code review:

- relogin.cgi?action=prepare-sudo : I can still enter any random password to impersonate a user.

- Bugzilla::Auth::Verify doesn't know what validate_email_syntax() is because "use Bugzilla::Util" is missing:
Undefined subroutine &Bugzilla::Auth::Verify::validate_email_syntax called at Bugzilla/Auth/Verify.pm line 75.

- When fixing the problem above, I get, using the ENV method:
Can't call method "disabledtext" on an undefined value at Bugzilla/Auth.pm line 94.

- The "Log Out" link in the footer appears with the "Env,CGI" combination, even when I was logged in using the ENV method. Clicking this link doesn't help. I first get a screen which says I'm logged out, but on the next action, the footer redisplays that I'm still logged in.

- userprefs.cgi?tab=account allows me to change my email address?!


Looks like you have to store somewhere the method used to authenticate, and then only look at this method to know what can be done, instead of looping over all available methods, including those you didn't use to log in.

Comment 20

[Max Kanat-Alexander]

Attachment: v6

Okay, the code is now much more sane. I removed the distinction between _account_source and _verifier, and everything makes much more sense, now.

Also, I put "Cookie" at the bottom of the stack, instead of at the top, for the Info Getters. That should fix pretty much all the bugs above. Also, I fixed the way that a lot of the can_ functions work inside Auth.pm itself.

Comment 21

[Frédéric Buclin]

Comment on attachment 221735 [details] [diff] [review]
v6

Again tests only:

- With both user_info_class="Env,CGI" and "CGI" methods, I cannot change my password nor my email address, despite allowemailchange=1.

- Using the CGI method, relogin.cgi?action=prepare-sudo still let me impersonate a user if I enter no password at all (an incorrect password is correctly handled).

- When using the ENV method, show_bug.cgi asks me again to authenticate despite I'm already authenticated. Other pages do not have this strange behavior.


All other comments I reported previously are fixed.

Comment 22

[Max Kanat-Alexander]

Attachment: v7

Okay, I fixed the first two. We've established that the third one is something the current code already does; we'll have to look into it.

Comment 23

[Max Kanat-Alexander]

Attachment: v7.1

This version also correctly removes Bugzilla/Auth/README

Comment 24

[Frédéric Buclin]

Comment on attachment 221753 [details] [diff] [review]
v7.1

Remaining broken things:

- relogin.cgi?action=prepare-sudo displays the password field with the ENV method when user_info_class="Env,CGI". The code doesn't care about it though.

- userprefs.cgi?tab=account allows you to change the password and email address of a user being impersonated when using the ENV method when user_info_class="Env,CGI".

- show_bug.cgi asks again to authenticate with the ENV method. It seems it does so only when the user hasn't enough privs to access the bug (it doesn't ask if you already have enough privs IIRC).

- userprefs.cgi?tab=account displays the "old password" field in all cases, which is pretty useless with the ENV method as you cannot edit anything except your realname (which doesn't require your old password anyway).


I looked at this patch pretty quickly as it seems very similar to the one I reviewed carefully a few days ago. I think we should now land this patch and fix problems mentioned above in separate bugs. This is too much work to reread this huge patch again and again.

Note two things:

- 012throwables.t will complain that there are two unused error tags. We opened bug 337444 about this problem already. tinderboxes will burn a few days. That's not a big problem.

- We should send an email to developers@ to warn them that the auth system must be considered as *highly unsecure and buggy*. This is a complete rearch of the auth stuff, we must expect regressions and even security issues, eventually.

So based on comments above: r=LpSolit

Comment 25

[Frédéric Buclin]

Do not forget to remove Bugzilla/Auth/Login/WWW on checkin.

Comment 26

[Mark McCallister]

I'm authenticating against an Active Directory Server. Authentication/Login works fine in HEAD. After applying v6 or v7.1 of your patch, I get the following error when logging in as an ldap user:

[Thu May 11 16:39:01 2006] [error] [client X.X.X.X] [Thu May 11 16:39:01 2006] index.cgi: Can't locate object method "realname" via package "Bugzilla::User" at Bugzilla/Auth/Verify.pm line 112, <DATA> line 283., referer: http://somebugzillahost/index.cgi?GoAheadAndLogIn=1

Comment 27

[Max Kanat-Alexander]

(In reply to comment #26)
> I'm authenticating against an Active Directory Server. Authentication/Login
> works fine in HEAD. After applying v6 or v7.1 of your patch, I get the
> following error when logging in as an ldap user:

  Okay. Could you file that as a new bug and assign it to me? I definitely want to keep LDAP working.

Comment 28

[Max Kanat-Alexander]

Hooray!! :-)

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.31; previous revision: 1.30
done
Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.160; previous revision: 1.159
done
Checking in relogin.cgi;
/cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v  <--  relogin.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.41; previous revision: 1.40
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.41; previous revision: 1.40
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.97; previous revision: 1.96
done
Checking in Bugzilla/Auth.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth.pm,v  <--  Auth.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.38; previous revision: 1.37
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.108; previous revision: 1.107
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login.pm,v
done
Checking in Bugzilla/Auth/Login.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login.pm,v  <--  Login.pm
initial revision: 1.1
done
Removing Bugzilla/Auth/README;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/README,v  <--  README
new revision: delete; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify.pm,v
done
Checking in Bugzilla/Auth/Verify.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify.pm,v  <--  Verify.pm
initial revision: 1.1
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Cookie.pm,v
done
Checking in Bugzilla/Auth/Login/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Cookie.pm,v  <--  Cookie.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Env.pm,v
done
Checking in Bugzilla/Auth/Login/Env.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Env.pm,v  <--  Env.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Stack.pm,v
done
Checking in Bugzilla/Auth/Login/Stack.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Stack.pm,v  <--  Stack.pminitial revision: 1.1
done
Removing Bugzilla/Auth/Login/WWW.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW.pm,v  <--  WWW.pm
new revision: delete; previous revision: 1.8
done
Removing Bugzilla/Auth/Login/WWW/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/CGI.pm,v  <--  CGI.pmnew revision: delete; previous revision: 1.17
done
Removing Bugzilla/Auth/Login/WWW/Env.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/Env.pm,v  <--  Env.pmnew revision: delete; previous revision: 1.7
done
Removing Bugzilla/Auth/Login/WWW/CGI/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/CGI/Cookie.pm,v  <--  Cookie.pm
new revision: delete; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v
done
Checking in Bugzilla/Auth/Persist/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v  <--  Cookie.pm
initial revision: 1.1
done
Checking in Bugzilla/Auth/Verify/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/DB.pm,v  <--  DB.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Auth/Verify/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm,v  <--  LDAP.pm
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/Stack.pm,v
done
Checking in Bugzilla/Auth/Verify/Stack.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/Stack.pm,v  <--  Stack.pm
initial revision: 1.1
done
Checking in template/en/default/index.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/index.html.tmpl,v  <--  index.html.tmpl
new revision: 1.25; previous revision: 1.24
done
Checking in template/en/default/sidebar.xul.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/sidebar.xul.tmpl,v  <-- sidebar.xul.tmpl
new revision: 1.21; previous revision: 1.20
done
Removing template/en/default/account/auth/ldap-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/ldap-error.html.tmpl,v  <--  ldap-error.html.tmpl
new revision: delete; previous revision: 1.4
done
Checking in template/en/default/account/auth/login-small.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login-small.html.tmpl,v  <--  login-small.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/account/auth/login.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login.html.tmpl,v  <--  login.html.tmpl
new revision: 1.15; previous revision: 1.14
done
Checking in template/en/default/account/prefs/account.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/account.html.tmpl,v  <--  account.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/admin/sudo.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/sudo.html.tmpl,v  <--  sudo.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.65; previous revision: 1.64
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v  <--  useful-links.html.tmpl
new revision: 1.44; previous revision: 1.43
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.168; previous revision: 1.167
done

Comment 29

[Frédéric Buclin]

I opened separate bugs for the first 3 comments from comment 24. The fourth one is less critical. You can find these bugs using the dependency tree.

Comment 30

[Max Kanat-Alexander]

Added to the release notes on bug 349423.