180642 - Move authentication code into a module

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Bradley Baetz (:bbaetz)]

quictly_check_login and confirm_login need to move into a module. I'm thinking
of an API of:

package Bugzilla::Auth;

sub auth_from_cgi {
  my ($cgi, $required) = @_;
  ...
  {
    no strict 'refs';
    require Param('authModule');
    my $ok = Param('authModule')::authenticate($userid, $pw); # Or whatever the
syntax is
  }
  ...
  $cgi->add_cookie(...);
  ...
  # Backwards compat wrapper will just return the userid.
  return new Bugzilla::User($userid);
}

We then have a Bugzilla::Auth::DB and Bugzilla::Auth::LDAP, with at least
B::A::DB defined in the same file for simplicity.

Or I could just call it |auth|, and either check the isa of the first argument,
or use auth_from_foo for the other (less common) forms of authentication.

Thoughts?

Comment 1

[Joel Peshkin]

In the process of doing this, we should make it possible to force users to log
in for everything (probably a param).  

Comment 2

[Bradley Baetz (:bbaetz)]

I'm finishing testing a patch now which I'll document, and then attach a draft.

I can't test LDAP, though, so I'll need help testing. ccing people how have
commented on/fixed LDAP bugs in the past. (Most of those haven't been checked
in; hopefully this bug will make it easier to do so without affecting the rest
of the product)

The LDAP code is ugly, though, and if someone wants to rewrite it (using
Net::LDAP in the process, like bugs 158630 and 120128 do, and/or fixing any of
the other enhancements in the meantime), that would be good. I haven't even
moved it away from PutHeader/PutFooter - I just moved the code basically as-is.

Do _NOT_ run the patch on a production installation. For starters, I still need
to read through it to check that I haven't left any security holes.

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, here we go.

24 files changed, 964 insertions(+), 462 deletions(-)

About 120 lines of the added code is licence stuff, though, and 210 are pod
comments. An additional 50 is just plain moved stuff (eg stuff in checksetup).
(That makes the comments arround 20% of the 'real' bits of the patch. Fun)

The rest is not so much code movement as code rewrite, so this will need very
careful review. On the plus side, its a _lot_ cleaner, and a lot more
extendable.

As I mentioned before, the LDAP stuff is totally untested. It compiles if I
comment out the |use Mozilla::LDAP;|, and thats it.

Thisneed careful viewing for security. A couple of things I changed:

- We don't CheckEmailSyntax the given name. The db will tell us if its correct
or not
- We no longer check for the dbwritesok state when updating stuff from login
handlers. Login handlers must be called from a writable database because of the
mirroring delay - if you log into the shadowdb, then your cookie form themain
db may not have popogated, and so on. (Maybe I should assert that somewhere?)
- I didn't change the cookie code. Some of it should be redone - the TO_DAYS
SQL especially, which has another bug covering that.
- The login cookie now holds a userid, not a username. This was easier, because
the auth code doesn't have access to the email, without looking it up, or
something. I also removed some cruft which wasn't used but seemed to indicate
that at one stage the password was part of the cookie...
- I removed some unreachable code which tried to insert a new user if you
logged in. This was unreachable, but was still useless.
- Other stuff I've probably forgotton.

Anyway, perldoc Bugzilla::Auth should show whats happening.

Again, LDAP help on a _test_ instance of Bugzilla would be nice to make sure
that it still works.

Comment 4

[Dave Miller [:justdave]]

ok, this is now installed on http://landfill.bugzilla.org/bz-ldap/

openldap on landfill is configured and has loaded the local password database
from landfill's shell.

Unfortunately, checksetup now fails with the following:

Can't locate Mozilla/LDAP.pm in @INC (@INC contains:
/usr/lib/perl5/5.6.1/i386-linux /usr/lib/perl5/5.6.1
/usr/lib/perl5/site_perl/5.6.1/i386-linux /usr/lib/perl5/site_perl/5.6.1
/usr/lib/perl5/site_perl/5.6.0/i386-linux /usr/lib/perl5/site_perl/5.6.0
/usr/lib/perl5/site_perl .) at Bugzilla/Auth/LDAP.pm line 36.

nice polite version check we do before trying to use it.  :-)

Next problem:
cpan> install Mozilla::LDAP
Warning: Cannot install Mozilla::LDAP, don't know what it is.

Comment 5

[Dave Miller [:justdave]]

ok, found Mozilla::LDAP.  it's in CPAN, it's just not indexed right :)

But it won't compile against the OpenLDAP libraries, it definitely requires
specifically the Netscape/iPlanet LDAP libraries.

I'm installing Net::LDAP right now, probably as good an excuse as any to get it
working with that. :)

Comment 6

[Gervase Markham [:gerv]]

Mozilla::LDAP is PerLDAP:
http://www.mozilla.org/directory/perldap.html

However, several people in the newsgroups who have got LDAP working have used
Net::LDAP. Given that this seems to be the mainstream of LDAP Perl client
development, would it not be more sensible to migrate across? Check back
newsgroup posts for stories about this; I seem to remember it didn't require too
much change.

Gerv

Comment 7

[Bradley Baetz (:bbaetz)]

There are already Net::LDAP patches - see bug 120128 and bug 158630. I just
wanted one thing at a time. That module needs to be redone from scratch, now
thats its api is simpler, anyway.

There arne't any sanity checks, because the current code doens't have them. I
should probbaly have teh param changing code in editparams test for it, or
something.

Comment 8

[Dave Miller [:justdave]]

Attachment: v2

Save as v1, but the LDAP.pm module has been rewritten to use Net::LDAP, and
it's actualy been tested on landfill and it works :)

Comment 9

[Dave Miller [:justdave]]

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

tested this on landfill, DB version works, LDAP version has issues.  I fixed
most of the LDAP issues in the new patch

Comment 10

[Dave Miller [:justdave]]

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

carrying forward my r= from the previous patch for all of bbaetz's work.

obviously we'll need someone else, too, since I added code to it.

Comment 11

[Dave Miller [:justdave]]

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

actually, I changed my mind, let's call this a needs-work still :)

checksetup.pl should fail gracefully instead of crashing if LDAP auth is chosen
in params when its run.  Should probably version check for Net::LDAP, too
(since we do version check for Graphviz when it's selected, so there's
precident).

A really minor nit, but it would be cool if the Auth module had a function that
would automatically return a list of all of the .pm files it didn't use for
other purposes in the Auth folder, and that list got used in defparams.  That
way if someone writes a new Auth module all they have to do is drop it in,
instead of having to edit defparams.pl to add the listing for it.

Comment 12

[Bradley Baetz (:bbaetz)]

Attachment: Take 3

Note that we won't warn if you clear out the server name after LDAP is enabled.
This is because of ordering - see comments in the code for more details.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 114524 [details] [diff] [review]
Take 3

This patch does not contain Auth.pm or any of the contents of the Auth
directory, which I assume are the main points of posting the patch, since
that's what you talked about changing...

Comment 14

[Bradley Baetz (:bbaetz)]

Attachment: v3a

Um, yeah, oops. Silly -N option...

Comment 15

[Tobias Burnus]

At least when bug 135820 had been checked in,
+    Token::DeletePasswordTokens("user logged in");
should be changed to use 'user_logged_in'.

Comment 16

[Jacob Steenhagen]

Comment on attachment 114539 [details] [diff] [review]
v3a

>Index: Bugzilla/Auth.pm
>===================================================================
...
>+=item C<AUTH_LOGINFAILED>
>+
>+An incorrect username or password was given. Note that for security reasons,
>+there is no distinction between these two cases.
>+
>+The second argument in the returned array is the userid, if known, and the
>+third argument is an optional string from the authentication server describing
>+the error.

The second sentence of the first paragraph doesn't seem to be entirely true.
When reading Bugzilla::Auth::DB it appears that if the user ID exists but the
password isn't correct it will still return the userid.

>Index: Bugzilla/DB.pm
>===================================================================

The changes to this file don't seem to have anything to do with this patch...
at least AFAICT.

>Index: Bugzilla/User.pm
>===================================================================

This one, too... but it's just a comment :)

>Index: Bugzilla/Auth/Cookie.pm
>===================================================================
...
>+    if (defined $netaddr) {
>+        trick_taint($netaddr);
>+        $query .= " OR logincookies.ipaddr=?";
>+    }

I assume that the ? substitution thing is smart enough to not care that we're
passing it more variables than there are ?'s if $netaddr is not defined...

>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare($query);
>+    my ($userid, $disabledtext) = $dbh->selectrow_array($query, undef,
>+                                                        $login_cookie,
>+                                                        $login,
>+                                                        $ipaddr,
>+                                                        $netaddr);

This seems wrong... why do we set up a $sth handler and then call
$dbh->selectrow_array()?

Please note: This is based on a read-through of the code, not on any actual
testing...

Comment 17

[Bradley Baetz (:bbaetz)]

> The second sentence of the first paragraph doesn't seem to be entirely true.

Wel, thers no distinction in the return value, and some of the modules don't
make a difference. Suggestions for better wording?

> I assume that the ? substitution thing is smart enough to not care that we're
> passing it more variables than there are ?'s if $netaddr is not defined...

Right, because teh extra args are actually an array, and we pass in undef.

> This seems wrong... why do we set up a $sth handler and then call
> $dbh->selectrow_array()?

Err, good catch. The first arg should be $sth instead (the select* stuff can
take a statement handle or a string)

Comment 18

[Jacob Steenhagen]

> Wel, thers no distinction in the return value, and some of the modules don't
> make a difference. Suggestions for better wording?

That's the real trick, isn't it... maybe something like:

=item C<AUTH_LOGINFAILED>

An incorrect username or password was given. Note that for security reasons,
both cases return the same error code; however, in the case of a valid username
but an invalid password, the userid is still returned in case the calling module
needs to be able to distinguish between the two cases. Passing this information
back to the user is not recommended.

Comment 19

[Bradley Baetz (:bbaetz)]

Attachment: v4

The DB.pm stuff is useful for showing the exact error string, which is useful
for error messages and the like.

I'll make the trivial change in comment #15 if that goes in before this.

Comment 20

[Bradley Baetz (:bbaetz)]

Attachment: v4a

Fixes cvs conflicts, no other changes

Comment 21

[Dave Miller [:justdave]]

Comment on attachment 115238 [details] [diff] [review]
v4a

>Index: editusers.cgi

>+    if(Bugzilla::Auth->canedit) {
>+      print "The authentication mechanism you are using does not permit accounts to be created.";

>+    unless (Bugzilla::Auth->can_edit) {
>+      print "This site's authentication mechanism does not allow new users to be added.";

Maybe these should specify "via Bugzilla" so we aren't sounding like they
really can't do it, just advising that they have to go to the external source
to do it.

Did we get the "user logged in" -> "user_logged_in" thing?

That's all my comments for the moment, from visual inspection.	I'll add more
after I've tried it out again.

Comment 22

[Bradley Baetz (:bbaetz)]

OK, I'll change that

Yes, we got the _ thing that was part of the cvs conflict

Comment 23

[Dave Miller [:justdave]]

Comment on attachment 115238 [details] [diff] [review]
v4a

works pretty good.

My only complaints outside of my previous comments is that the "New Account"
link in the footer is still present when using an authentication type which
doesn't allow account creation.  That and the "create an account" link on the
login page should probably just go away in those circumstances.  I'd leave the
Add link in editusers, so the admin can get a clue where he has to go to add
users :)

Comment 24

[Bradley Baetz (:bbaetz)]

I spoke to justdave on irc about his last comments. Doing that is a pain at the
moment, since the Auth stuff would have to be globally accessible from the
template to be present in the footer. That would make the DEFAULT stuff in
Bugzilla::Template depend on ::Auth, but ::Auth depends on ::Template for error
handling, and so things arne't very happy.

It may be possible to fix this after error handling is cleaned up, but for the
moment he's agreed that that should be a separate bug.

I locally fixed the other sissue wrt the error message (and also changed
->canedit to ->can_edit; oops)

Comment 25

[Bradley Baetz (:bbaetz)]

Comment on attachment 115238 [details] [diff] [review]
v4a

justdave: Given that we decided that your other issues are for a new bug, can
you r= this?

gerv, do you have time for this?

Comment 26

[Gervase Markham [:gerv]]

%&$*@! I just finished a nice long review in a mail message, copied it all to
clipboard, opened the attachment edit window, clicked "Edit attachment as
comment" so I could paste it in, and bang! Mozilla fell over. No core dump file,
no auto-save of my draft (_why_ do we not do that? Outlook does it, and it
hardly ever crashes), nothing. And it cleaned out the clipboard as well, so I
couldn't paste it into another application. Fantastic.

I'll redo it, but the comments might be a little less polite and a little more
terse this time round :-)

Gerv

Comment 27

[Gervase Markham [:gerv]]

Comment on attachment 115238 [details] [diff] [review]
v4a

> Index: Bugzilla.pm
> ===================================================================
> +sub login {
> +    my ($class, $type) = @_;
> +
> +    # Avoid double-logins, which may confuse the auth code
> +    # (double cookies, odd compat code settings, etc)
> +    # This is particularly important given the munging for
> +    # $::COOKIE{'Bugzilla_login'} from a userid to a loginname
> +    # (for backwards compat)
> +    if (defined $_user) {
> +        return $_user->{id};
> +    }

This returns a userid...

> +        # Evil compat hack. The cookie stores the id now, not the name, but
> +        # old code still looks at this to get the current user's email
> +        # so it needs to be set.
> +        $::COOKIE{'Bugzilla_login'} = $_user->{email};

Why not change the cookie name if the contents are changing? Makes life much
simpler.

> +=item C<login>
> +
> +Logs in a user, returning the L<Bugzilla::User> object (if any). See

It actually returns a userid.

> Index: CGI.pl
> ===================================================================
>  sub quietly_check_login {
> +    return Bugzilla->login($_[0] ? LOGIN_OPTIONAL : LOGIN_NORMAL);
>  }

So is this sort of code going to eventually appear in all CGIs, rather than
q_c_l or c_l?

> Index: Token.pm
> ===================================================================
> RCS file: /cvsroot/mozilla/webtools/bugzilla/Token.pm,v
> retrieving revision 1.16
> diff -u -r1.16 Token.pm
> --- Token.pm	30 Sep 2002 07:22:39 -0000	1.16
> +++ Token.pm	22 Feb 2003 14:12:02 -0000
> @@ -240,16 +240,17 @@
>      &::SendSQL("UNLOCK TABLES");
>  }
>  
> -sub HasPasswordToken {
> -    # Returns a password token if the user has one.
> -    
> -    my ($userid) = @_;
> -    
> -    &::SendSQL("SELECT token FROM tokens 
> -                WHERE userid = $userid AND tokentype = 'password' LIMIT 1");
> -    my ($token) = &::FetchSQLData();
> -    
> -    return $token;
> +sub DeletePasswordTokens {
> +    my ($userid, $reason) = @_;
> +
> +    my $dbh = Bugzilla->dbh;
> +    my $sth = $dbh->prepare("SELECT token " .
> +                            "FROM tokens " .
> +                            "WHERE userid=? AND tokentype='password'");
> +    $sth->execute($userid);
> +    while (my $token = $sth->fetchrow_array) {
> +        Token::Cancel($token, "user_logged_in");
> +    }
>  }

What are these changes for?

> Index: defparams.pl
> ===================================================================
> RCS file: /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v
> retrieving revision 1.110
> diff -u -r1.110 defparams.pl
> --- defparams.pl	23 Jan 2003 23:34:00 -0000	1.110
> +++ defparams.pl	22 Feb 2003 14:12:06 -0000
> @@ -123,6 +123,28 @@
>      return "";
>  }
>  
> +sub check_loginmethod {
> +    # This checks that parameters are present. If they're cleared later, though
> +    # then we won't error out. This is because clearing would depend on this
> +    # param, while this check depends on earlier params (loginmethod is
> +    # after the server params) and we process everything in one pass ATM

This comment is very cryptic; I don't really understand it. Please clarify :-)

>    {
> +   name => 'LDAPbinddn',
> +   desc => 'If your LDAP server requires that you use a binddn and password ' .
> +           'instead of binding anonymously, enter it here ' .
> +           '(e.g. cn=default,cn=user:password). ' .
> +           'Leave this empty for the normal case of an anonymous bind.',
> +   type => 't',
> +   default => ''
> +  },
> +

Isn't there a thing called an LDAP URL we could use and parse to make this one
parameter instead of four for LDAP?

> +  {
> +   name => 'loginmethod',
> +   desc => 'The type of login authentication to use',
> +   type => 's',
> +   choices => [ 'DB', 'LDAP' ],
> +   default => 'DB',
> +   checker => \&check_loginmethod
>    },

"DB" is a very obscure name for the standard authentication. The underlying
problem here is that you've bound the values in this dropdown to module names
directly, which I think is a mistake. It's a localisation problem too, a little
further out. I suggest you decouple them, and have the displayed value as
something like "Internal Bugzilla Authentication" or "Standard Bugzilla" or
"Local Internal Database" or similar.

> Index: editusers.cgi
> ===================================================================
> RCS file: /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v
> retrieving revision 1.40
> diff -u -r1.40 editusers.cgi
> --- editusers.cgi	22 Sep 2002 17:14:53 -0000	1.40
> +++ editusers.cgi	22 Feb 2003 14:12:06 -0000
> @@ -110,8 +110,8 @@
>      if ($editall) {
>          print "</TR><TR>\n";
>          print "  <TH ALIGN=\"right\">Password:</TH>\n";
> -        if(Param('useLDAP')) {
> -          print "  <TD><FONT COLOR=RED>This site is using LDAP for authentication!</FONT></TD>\n";
> +        if(Bugzilla::Auth->can_edit) {
> +          print "  <TD><FONT COLOR=RED>This site's authentication method does not allow password changes!</FONT></TD>\n";

You need to carefully check the sense of all of the can_edit checks in this
file; I really think half of them are wrong. Take this one: "If I can edit,
then print a message saying I can't." If that's really correct, it needs
commenting.


> +Handles authentication for Bugzilla users.
> +
> +Authentication from Bugzilla involves two sets of modules. One set is used to
> +obtain the data (from CGI, email, etc), and the other set uses this data to
> +authenticate against the datasource (the Bugzilla DB, LDAP, cookies, etc).
> +
> +The handlers for the various types of authentication (Cookie, LDAP, etc)
> +provide the actual code for each specific method of authentication.

You use different examples in the two paras, which is confusing. Define a set
of terms in the first para, then use them consistently.

> +
> +The source modules (currently, only L<Bugzilla::Auth::CGI>) then use those
> +methods to do the authentication.
> +
> +I<Bugzilla::Auth> itsself inherits from the default authentication handler,
> +identified by the I<loginmethod> param.

"itself". I found about eight typos/spelling mistakes in the POD first time
through; I may not find them all this time, but you should proofread it
carefully.

> +C<Bugzilla::Auth> contains several helper methods to be used by
> +authentication or login modules.
> +
> +=over 4
> +
> +=item C<Bugzilla::Auth::get_netaddr($ipaddr)>
> +
> +Given an ip address, this returns the associates netmask, using
> +C<Param('loginnetmask')>. This can be used to obtain data in order to restrict
> +weak authentication methods (such as cookies) to only some addresses.

Can we take the opportunity to change the name to "get_netmask", then? The name
and the description of what it does don't match.

"associated netmask". 

> +This method is passed a username and a password, and returns a list containing
> +up to three variables, depending on the results of the authentication

Nit: missing full stop.

> +The first variable is a status argument, which is a constant from
> +L<Bugzilla::Constants>. The contents of the other variabels are described
> +below.

"variables".

> +=item C<AUTH_ERROR>
> +
> +An error occurred when trying to use the login mechanism. The second return
> +value may contain the Bugzilla userid, but will probably be C<undef>,
> +signifiying that the userid is unknown The
> +third value is a string describing the error.

Please use error IDs or tags rather than strings - these can then be translated
into printable strings using templates or some other mechanism. This lot has to
be localised eventually, and it's best to not change interfaces when doing it.

Same for the rest of these return interfaces.

> +=item C<AUTH_LOGINFAILED>
> +
> +An incorrect username or password was given. Note that for security reasons,
> +both cases return the same error code. However, in the case of a valid
> +username, the second argument may be the userid. The authenication mechanism

"authentication".

> +may not always be able to discover the userid if the password is not known,
> +so whether or not this argument is present is implementation defined. For
> +security reasons, the presense or lack of a userid value should not be
> +communicated to the user.

"presence".

> +The user successfully logged in, but their account has been disabled. The
> +second argument in the returned array is the userid, and the third is some
> +text explaining why the account was disabled. This text would typically come
> +from the C<disabledtext> field in the C<profiles> table.

But this is OK, because presumably it'll be in the local language.

> +This determines if the user's account details can be modified. If this method
> +returns a C<true> value, then accounts can be created and modified through
> +the Bugzilla user interface. Forgotton passwords can also be retreived through
> +the L<Token> interface.

"Forgotten". "retrieved".

> +A login module can be used to try to login a user who is using Bugzilla in a

"to log a user in". Verbing weirds language.

> +particualar way. For example, L<Bugzilla::Auth::CGI> logs in users from CGI

"particular".

> +=item C<LOGIN_OPTIONAL>
> +
> +A login is never required to access this data.

Then why auth at all? :-) You might want to add a note here explaining that
auth still makes sense because you may want to personalise the page.

> +A login is always requred to access this data.

"required".

> Index: Bugzilla/DB.pm
> ===================================================================
> RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v
> retrieving revision 1.7
> diff -u -r1.7 DB.pm
> --- Bugzilla/DB.pm	20 Feb 2003 07:07:42 -0000	1.7
> +++ Bugzilla/DB.pm	22 Feb 2003 14:12:07 -0000
> @@ -61,8 +61,6 @@
>  sub SendSQL {
>      my ($str) = @_;
>  
> -    require Bugzilla;
> -
>      $_current_sth = Bugzilla->dbh->prepare($str);
>  
>      $_current_sth->execute;
> @@ -79,8 +77,6 @@
>      # Backwards compat code
>      return "''" if not defined $str;
>  
> -    require Bugzilla;
> -
>      my $res = Bugzilla->dbh->quote($str);
>  
>      trick_taint($res);
> @@ -156,6 +152,7 @@
>                             $db_pass,
>                             { RaiseError => 1,
>                               PrintError => 0,
> +                             ShowErrorStatement => 1,
>                               HandleError => \&_handle_error,
>                               FetchHashKeyName => 'NAME_lc',
>                               TaintIn => 1,

How are these changes related to this patch?

> Index: Bugzilla/Auth/CGI.pm
> ===================================================================
> +# Contributor(s): Terry Weissman <[e-mail]>
> +#                 Dan Mosedale <[e-mail]>
> +#                 Joe Robins <[e-mail]>
> +#                 Dave Miller <[e-mail]>
> +#                 Christopher Aillon <[e-mail]>
> +#                 Gervase Markham <[e-mail]>
> +#                 Christian Reis <[e-mail]>
> +#                 Bradley Baetz <[e-mail]>

For this file and others, you should do some research to see who actually
touched this code. Otherwise, as code moves around, contributor lists grow
without bound, and don't reflect reality. 

> +    my ($authres, $userid, $extra) =
> +      Bugzilla::Auth->authenticate($username, $passwd);

Hmm. Do we right justify split-lines, or indent? This looks odd...

> +        # This seems like as good as time as any to get rid of old
> +        # crufty junk in the logincookies table.  Get rid of any entry
> +        # that hasn't been used in a month.
> +        Bugzilla->dbh->do("DELETE FROM logincookies " .
> +                          "WHERE TO_DAYS(NOW()) - TO_DAYS(lastused) > 30");

We do this too often. Is there any way to do it less often but regularly - e.g.
if time() ends in 00, or something?

> +    # An error may have occurred with the login mechanism
> +    if ($authres == AUTH_ERROR) {
> +        &::ThrowCodeError("auth_error",
> +                          { 'userid' => $userid,
> +                            'errorstring' => $extra,
> +                          }
> +                         );

We normally format ThrowCodeError calls with embedded hashes as:
&::ThrowCodeError("auth_error",
		  { 'userid' => $userid, 'errorstring' => $extra });
or
&::ThrowCodeError("auth_error", { 'userid' => $userid, 
				  'errorstring' => $extra });

> +    # The account may be disabled
> +    if ($authres == AUTH_DISABLED) {
> +        # Clear the cookie
> +        my $cookiepath = Param("cookiepath");
> +        print "Set-Cookie: Bugzilla_login= ; path=$cookiepath; expires=Sun, 30-Jun-80 00:00:00 GMT
> +Set-Cookie: Bugzilla_logincookie= ; path=$cookiepath; expires=Sun, 30-Jun-80 00:00:00 GMT
> +Content-type: text/html
> +
> +";
> +        # and throw a user error
> +        &::ThrowUserError("account_disabled",
> +                          {'disabled_reason' => $extra});

This is going to cause nasty multiple-printings of Content-Types. Why print the
CT here at all?

> +If an incorrect username/password is given, then the user is 
> +
> +=head1 SEE ALSO

"The user is"... what? :-)

> Index: Bugzilla/Auth/Cookie.pm
> ===================================================================
> +    my $dbh = Bugzilla->dbh;
> +    my $sth = $dbh->prepare($query);
> +    my ($userid, $disabledtext) = $dbh->selectrow_array($sth, undef,
> +                                                        $login_cookie,
> +                                                        $login,
> +                                                        $ipaddr,
> +                                                        $netaddr);

dbh/sth confusion. If this isn't the one already pointed out, then you should
check the entire patch carefully, as you may have done this more times.

> +The actual password is not stored in the cookie; only the userid and a
> +I<cookie> are. 

A userid and a cookie are stored in a cookie? Please correct or explain :-)

> Index: Bugzilla/Auth/DB.pm
> ===================================================================
> +    my $sth = $dbh->prepare_cached("SELECT userid,cryptpassword,disabledtext " .
> +                                   "FROM profiles " .
> +                                   "WHERE login_name=?");
> +    my ($userid, $realcryptpwd, $disabledtext) =
> +      $dbh->selectrow_array($sth,
> +                            undef,
> +                            $username);

What's the difference between this and calling selectrow_array on the $sth?

> Index: Bugzilla/Auth/LDAP.pm
> ===================================================================
> +    my $userEntry = $mesg->shift_entry if !$mesg->code && $mesg->count;

Can we lose the StudlyCaps from this module? :-)

> Index: template/en/default/global/user-error.html.tmpl
> ===================================================================
> +  [% ELSIF error == "auth_cant_create_account" %]
> +    [% title = "Can't create accounts" %]
> +    This site is using an authentication scheme which does not permit
> +    account creation. Please contact an administrator to get a new account
> +    created.
> +

Should we use Param('maintainer') here?

Gerv

Comment 28

[Bradley Baetz (:bbaetz)]

> This returns a userid...

Comment changed. Returning a user object is the next step.

> Why not change the cookie name if the contents are changing? Makes life much
> simpler.

Because everywhere else references the cookie name. If they didn't, then I
wouldn't need that. Again, thats part of the User.pm round of fixes.

> So is this sort of code going to eventually appear in all CGIs, rather than
> q_c_l or c_l?

Yes, although the ?: stuff won't be needed. Also, LOGIN_NORMAL is the default,
so most scripts will only ned to call |Bugzilla->login();|, ie w/o a param.

> What are these changes for?

I had to change that code anyway, and it was easier to just delete everything in
one go, rather than one at a time. Quicker, too.

> This comment is very cryptic; I don't really understand it. Please clarify :-)

doeditparams traverses the list of params, and for each one it checks, then
updates. This means that if one param checker wants to look at other params, it
must be below that other one. So you can't have two params mutually dependant on
each other.

> Isn't there a thing called an LDAP URL we could use and parse to make this one
> parameter instead of four for LDAP?

No clue, but I'm not rewriting LDAP for this bug; someone else can do that

> "DB" is a very obscure name for the standard authentication.

The editparams code doesn't let me have a label different to the underlying
value, so I can't call it Database w/o changing the .pm name, which I don't want
to do.

> It's a localisation problem too, a little further out.

None of editparams is localisable, and when it is the name->description mapping
will be part of it.

> You need to carefully check the sense of all of the can_edit checks in this
> file

Oops. I'll go thrugh those. justdave tested the LDAP stuff on landfill, since
I'm not set up for that.

> Can we take the opportunity to change the name to "get_netmask", then?

OK

> Please use error IDs or tags rather than strings

Err, these are integer constants, not strings, defined in Bugzilla::Constants.
They can be exposed to the templates via a template plugin, similar to how the
Bugzilla stuff is done. (Actually, since they're constants, we could even use
the CONSTANT thing for templates)

Besides, they don't need to be translated. The results do, but the atual code
handles the erro code->error template mapping - it has to, given the different
required results for the various methods.

[Bugzilla::DB]
> How are these changes related to this patch?

They're not, but it means that we get the error sql on db errors, and it made it
easier for me to debug, and is probbaly useful anyway :)

> For this file and others, you should do some research to see who actually
> touched this code. Otherwise, as code moves around, contributor lists grow
> without bound, and don't reflect reality. 

Yeah, I thought about that. But since some people who have changed the code
aren't in teh list, and I'd hve to trace cvs annotate back through 232 revisions
 of globals.pl and 200 revisions of CGI.pl....

> We do this too often. Is there any way to do it less often but regularly -
> e.g. if time() ends in 00, or something?

Theres another bug for that (myk has this disbaed on bmo, and running via cron).
I want to change the schema so that its defined as expiry time, and then not
update it each run. If we do that, then being late to delete the cookie isn't a
security issue. But again, thast separate.

> We normally format ThrowCodeError calls with embedded hashes as:

It went over 80 cols.

> This is going to cause nasty multiple-printings of Content-Types. Why print
> the CT here at all?

Why will this cause multiple printings? If we're logging in, then the
runderlying script hasn't printed it.

> What's the difference between this and calling selectrow_array on the $sth?

prepare_cached keeps the sth arround. Under mod_perl, this is useful for stuff
(like this) which runs on every single login.

> Should we use Param('maintainer') here?

No; the bugzilla maintainer is not necessarily the ldap administrator.

I'll fix those up, and submit a new patch.

Comment 29

[Gervase Markham [:gerv]]

>> Why not change the cookie name if the contents are changing? Makes life much
>> simpler.
> 
> Because everywhere else references the cookie name. If they didn't, then I
> wouldn't need that. Again, thats part of the User.pm round of fixes.

No, I mean keep the old cookie name for the compat stuff (so everywhere which
uses it still works), but have the new cookie, which contains a userID and which
should only be looked at in a very small section of code, have a new name.

>> This comment is very cryptic; I don't really understand it. Please clarify :-)
> 
> doeditparams traverses the list of params, and for each one it checks, then
> updates. This means that if one param checker wants to look at other params, it
> must be below that other one. So you can't have two params mutually dependant on
> each other.

Right. But I mean, of course, "please clarify in a comment" :-)

>> "DB" is a very obscure name for the standard authentication.
> 
> The editparams code doesn't let me have a label different to the underlying
> value, so I can't call it Database w/o changing the .pm name, which I don't want
> to do.

Well you could, if you didn't make it so that the label mapped directly to the
.pm name. :-) I.e. when you loaded the relevant .pm, you had a lookup table.

>> Please use error IDs or tags rather than strings
> 
> Err, these are integer constants, not strings, defined in Bugzilla::Constants.

I meant the third parameter, not the first, which is defined in the POD as "a
string describing the error". It should be "a tag which uniquely identifies the
error" :-)

>> For this file and others, you should do some research to see who actually
>> touched this code. Otherwise, as code moves around, contributor lists grow
>> without bound, and don't reflect reality. 
> 
> Yeah, I thought about that. But since some people who have changed the code
> aren't in teh list, and I'd hve to trace cvs annotate back through 232 revisions
>  of globals.pl and 200 revisions of CGI.pl....

It's not as much hassle as you make it sound, because many of those revisions
will be to other parts of the code. You go "Hmm. This large chunk was changed by
person X in revision 150. So X is a contributor. Now, let's look at 149. Hmm.
Seems that Y and Z changed it before that, in revisions 98 and 93. So let's look
at 97..."

>> We normally format ThrowCodeError calls with embedded hashes as:
> 
> It went over 80 cols.

There's several of them; I'm sure you can make at least some look more like the
standard :-) And we put the trailing ); on the same line as }. That can
certainly be managed.

>> This is going to cause nasty multiple-printings of Content-Types. Why print
>> the CT here at all?
> 
> Why will this cause multiple printings? 

Because you've printed it, and then the Throw*Error function will print it
again, because you haven't set header_done (although doing that wouldn't work
either.)

>> What's the difference between this and calling selectrow_array on the $sth?
> 
> prepare_cached keeps the sth arround. Under mod_perl, this is useful for stuff
> (like this) which runs on every single login.

No, I mean, what's the difference between:

$sth = whatever
and then

$foo = $sth->fetchrow_array(params);

and

$foo = $dbh->fetchrow_array($sth, undef, params);

?

Gerv

Comment 30

[Bradley Baetz (:bbaetz)]

> No, I mean keep the old cookie name for the compat stuff (so everywhere which
> uses it still works), but have the new cookie, which contains a userID and
> which should only be looked at in a very small section of code, have a new
> name.

Then we'd rely on the user sending all three cookies, so we'd have to validate
it anyway. Just think of it as compat code which will be removed later.

> Well you could, if you didn't make it so that the label mapped directly to the
> .pm name. :-) I.e. when you loaded the relevant .pm, you had a lookup table.

Well, yes.... But the idea is to reduce the number of places required to drop in
a new module. Localisation isn't an issue for this file at the moment, esp since
I've already mentioned how it can be done. Not to mention that having a label
ins't localalisable anyway.

> I meant the third parameter, not the first, which is defined in the POD as "a
> string describing the error". It should be "a tag which uniquely identifies the
> error" :-)

No, its a string - it has to be, since it can be $::disabledtext, or an error
string from the Net::LDAP module. We can't localise that, since we don't control
it. Now, the Net::LDAP module doesn't actually use that yet, but it could, since
the LDAP server could return 'auth failure' vs 'expied password' vs 'something else'

> No, I mean, what's the difference between:

Absolutely nothing, unless you want to keep teh $sth arround for later. If you
look at the DBI source code, it calls ->prepare internally if the first arg
isn't a ref.

Comment 31

[Gervase Markham [:gerv]]

>> I meant the third parameter, not the first, which is defined in the POD as "a
>> string describing the error". It should be "a tag which uniquely identifies the
>> error" :-)
> 
> No, its a string - it has to be, since it can be $::disabledtext, or an error
> string from the Net::LDAP module. 

I understand that in the AUTH_DISABLED case, it has to be a string - but that's
fine, because the admin is responsible for writing one that the user can
understand :-) 

I also understand that, if the errors come back from another module, the "tag"
will actually be a long set of space-separated letter strings rather than an
underscore-separated tag that we use. However, we should use sensible tags even
if they don't.

To be more clear: we should define it as a tag, use things like
"cant_read_database" ourselves, and localisers will have to take the hit that
they may have to do

[% IF error.match("LDAP Error: can't read database:") %]
  Desole, mais je...
[% END %]

in their templates. But we can make it work right for our errors.

Gerv

Comment 32

[Bradley Baetz (:bbaetz)]

But it can't be a tag - the string comes back from the module. It could depend
on the server used, the LDAP module version, and so on.

Now, Net::LDAP has a whole host of possible error codes. But, Net::LDAP::Message
only uses those 'If the server did not include an error message'. I have
absolutly no idea whether or not most servers return a string, but I'd imagine
that they would.

I guess we could define a tag to mean 'use the next arg as is', but I'd rather
leave that until such time as someone rewrites LDAP (or another auth mothd) in
such a way that it matters. Lets get a user before we define the interface...

Comment 33

[Gervase Markham [:gerv]]

> But it can't be a tag - the string comes back from the module. It could depend
> on the server used, the LDAP module version, and so on.

So maybe those ones can't be translated, and will have to be passed through
straight, or just with some explanatory text.

My point is that your code currently does this:
+        return (AUTH_ERROR, undef, "LDAPserver param not defined");
and it shouldn't. I've spent the last five months eliminating this sort of thing
from Bugzilla, and I'd prefer it if you didn't add them back in again :-)

Gerv

Comment 34

[Bradley Baetz (:bbaetz)]

OK, I have an updated patch which uses tags; I just need to make the tags point
to something in a template...

I'm going to go form login-errors.html.tmpl for the internal ones - it puts them
all in one place for other modules to add to without worrying too much about
namespace collisions.

Comment 35

[Gervase Markham [:gerv]]

Is there a real need for another errors template? Currently, when looking for
particular errors, I have to check two templates (and sometimes open the wrong
one.) Having a third template will merely compound this problem.

For login errors, the standard ThrowUserError "Your login details are wrong yada
yada... Please press Back and try again." seems perfectly reasonable.

Gerv

Comment 36

[Bradley Baetz (:bbaetz)]

We can't use the standard error, because we have to report all those internal
errors which were previously just dies. Stuff like the LDAP param being missing,
not connecting to the LDAP server, and so on.

You won't have to search lots of places; only Bugzilla/Auth/*.pm.

Another template makes sense because:

a) Keeps namespacing separate - you won't have to look through these errors when
looking for other ones.
b) Its not like theres a limit to teh number of templates we can have.
c) A lot more flexible - the tag naming scheme will be <method>_<whatever>. We
have errors starting with email_, so if we ever have an email auth mechanism,
we'd have to be careful not to combine them.

Comment 37

[Dave Miller [:justdave]]

How about creating a new directory called auth?

Get rid of the existing login.html.tmpl that does the login screen, or move it
into the above directory.

Within that directory we can have either a directory for each auth module, or
preface the login screen and error template names with the name of the auth module.

I'd prefer to keep the data for each module in distinct files, so that in the
long run we CAN just drop in a new auth module and have it work.  Making someone
apply a patch to a template when they add a new module isn't exactly a nice
thing to do...

Comment 38

[Bradley Baetz (:bbaetz)]

OK, I have this working nicely now with tagged errors.

Gerv: If I want to reuse the existing code-error template, then I don't want to
print the vars at the bottom. There seems to be some internal inconsistancies
between messages using variables.*, and just using 'bare' variables. I'd lean
towards the latter - theres no point in printing the stuff out twice.
If you answer that, then I'll attach a final patch.

(I only tested the 'server not defined' error, since I don't actually have an
LDAP server arround to test with)

Comment 39

[Gervase Markham [:gerv]]

> There seems to be some internal inconsistancies
> between messages using variables.*, and just using 'bare' variables.

variables.*? What's that?

Gerv

Comment 40

[Bradley Baetz (:bbaetz)]

Its that extra hashref thingy passed into ThrowCodeError - see
code-error.html.tmpl, at the bottom

Comment 41

[Gervase Markham [:gerv]]

Oh, yes. I forgot those :-)

The distinction is that things which appear in the message get passed in as-is;
debugging information, such as the value of various internal variables, goes in
the variables hash.

Generally, you shouldn't be using the variables hash unless you are trying to
track down a particular heisenbug. So, you are right: use bare variables in
almost all cases.

Gerv

Comment 42

[Bradley Baetz (:bbaetz)]

/me looks at the graphing errors in that template :)

I'll tidy this up tomorrow/on the weekend, then

Comment 43

[Bradley Baetz (:bbaetz)]

Attachment: v5

OK, Gerv's issues fixed, including the login error stuff.

Justdave: can you test the LDAP on landfill again, please?

Comment 44

[Dave Miller [:justdave]]

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

LDAP Auth still works.	Nothing's jumping out at me here other than the login
screen gives no indication of what type of auth you're using, but that's a
cosmetic thing we can file another bug for if anyone cares. :)

Comment 45

[Dave Miller [:justdave]]

OK, I brought this up once before and got talked out of it, but I'm going to
bring it up again because I can't talk the people out of it that this is going
to affect....

For SecurID authentication for Zippy, we need three fields.  The username, their
password, and the current SecurID code off their keycard.  When I brought it up
before, I was told we should tack the keycode on the end of the password like
all the RSA stuff does by default.  Zippy won't go for it, they want all three
fields separate.

How about if we make the authentication plugins work like this:
The authentication module specifies a template to use for the login screen. 
When the form is submitted, the module, when asked to authenticate, is given
access to all the fields on the form.  Although since there's likely to be a lot
of hidden fields if they were submitting something else, perhaps give the auth
module every field that begins with a specific prefix (probably "Bugzilla_" if
we're keeping the existing field names).

Not only would this make what Zippy wants to do for SecurID workable, it would
make it a LOT more flexible in case of some other auth scheme down the road that
we haven't thought of yet, too.

Comment 46

[Dave Miller [:justdave]]

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

per my previous comments

Comment 47

[Myk Melez [:myk] [@mykmelez]]

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

>Index: Bugzilla.pm

>+    # For now, we can only login from a cgi

Nit: "log in"


>+    return $userid || 0;

Wouldn't it make more sense to return undefined if the user wasn't found?  Or
is 
zero what existing code currently expects?



>Index: Bugzilla/Auth.pm

>+=head1 AUTHENTICATION
>+
>+Authentication modules allow for a user's details to be checked for validity.

Urk.  "Authentication modules check a user's credentials (username, password)
to verify who the user is."


>+=item C<authenticate($username, $pass)>
>+
>+This method is passed a username and a password, and returns a list containing
>+up to three variables, depending on the results of the authentication.

Up to four in the case of AUTH_ERROR, and aren't they return values rather than
variables?


>+The first variable is a status argument, which is a constant from
>+L<Bugzilla::Constants>. The contents of the other variables are described
>+below.

Urk.  First, arguments are usually passed into a function, not out of one. 
Second, "which is a constant from L<Bugzilla::Constants>" makes no sense when
you list the constants right afterwards.  Third, "the contents of the other
variables are described below" is cryptic.  Say something like this instead:

"The first return value is one of the status codes defined in
Bugzilla::Constants and described below.  The rest of the return values are
status code-specific and are explained in the status code descriptions."


>+=item C<AUTH_OK>
>+
>+Authentication succeeded. The second variable is the userid for the new user.

Nit: s/for/of/


>+=item C<AUTH_ERROR>
>+
>+An error occurred when trying to use the login mechanism. The second return
>+value may contain the Bugzilla userid, but will probably be C<undef>,
>+signifiying that the userid is unknown. The third value is a tag describing
>+the error. The optional fourth argument is a hashref of values used as part
>+of the tag's error descriptions.

"The third value is a tag describing the error that the user-error.html.tmpl
template uses to determine which error message to print."


>+=item C<AUTH_LOGINFAILED>
>+
>+An incorrect username or password was given. Note that for security reasons,
>+both cases return the same error code. However, in the case of a valid
>+username, the second argument may be the userid. The authentication
>+mechanism may not always be able to discover the userid if the password is
>+not known, so whether or not this argument is present is implementation
>+defined.

Nit: implementation-specific.


>+=item C<can_edit>
>+
>+This determines if the user's account details can be modified. If this
>+method returns a C<true> value, then accounts can be created and modified
>+through the Bugzilla user interface. Forgotten passwords can also be
>+retrieved through the L<Token> interface.

On my system, perldoc converts "L<Token>" to "the Token manpage" (just as it
converted "L<Bugzilla::Constants>" to "the Bugzilla::Constants manpage" above).
 This makes the last sentence read "Forgotten passwords can also be retrieved
through the the Token manpage interface.", which is jibberish.	This should be
fixed.


>+A login module can be used to try to log a Bugzilla user in in a particular

Nit: "to log in a Bugzilla user in a particular"



>Index: Bugzilla/Config.pm

>+    if (exists $param{'useLDAP'} && !exists $param{'loginmethod'}) {
>+        $param{'loginmethod'} = $param{'useLDAP'} ? "LDAP" : "DB";
>+    }

How could useLDAP exist if you've removed it?



>Index: Bugzilla/Auth/CGI.pm

>+    # The account may be disabled
>+    if ($authres == AUTH_DISABLED) {
>+        # Clear the cookie
>+        my $cookiepath = Param("cookiepath");
>+        print "Set-Cookie: Bugzilla_login= ; path=$cookiepath; expires=Sun, 
30-Jun-80 00:00:00 GMT
>+Set-Cookie: Bugzilla_logincookie= ; path=$cookiepath; expires=Sun, 30-Jun-80 
00:00:00 GMT
>+
>+";

Nit: this would be better as single-line strings to preserve indentation, i.e.:

>+        print "Set-Cookie: Bugzilla_login= ; path=$cookiepath; expires=Sun, 
30-Jun-80 00:00:00 GMT\n" .
>+              "Set-Cookie: Bugzilla_logincookie= ; path=$cookiepath; 
expires=Sun, 30-Jun-80 00:00:00 GMT\n";

For that matter, why two line breaks at the end?  Doesn't ThrowUserError still 
need to print out the Content-Type header?



>Index: Bugzilla/Auth/Cookie.pm

>+    if (defined $netaddr) {
>+        trick_taint($netaddr);
>+        $query .= " OR logincookies.ipaddr=?";
>+    }
>+    $query .= ")";
>+
>+    my $dbh = Bugzilla->dbh;
>+    my ($userid, $disabledtext) = $dbh->selectrow_array($query, undef,
>+                                                        $login_cookie,
>+                                                        $login,
>+                                                        $ipaddr,
>+                                                        $netaddr);

If you are going to unconditionally pass $netaddr to the database handler
wouldn't it be good form to unconditionally trick_taint it?  This would also
make your code for adding it to the query string much simpler, i.e.: .
($netaddr ? " OR logincookies.ipaddr=?" : "") . ")";



>Index: Bugzilla/Auth/LDAP.pm

>+sub authenticate {
>+    my ($class, $username, $pass) = @_;

Nit: why not be consistent with DB.pm's use of "$passwd"?



Note that I haven't actually tested the LDAP code.  After upgrading, will
everyone have to log in all over again?  We should replace quietly_check_login
and confirm_login with the appropriate calls to Bugzilla::login.  I'll settle
for a bug on it.  Also, I agree with Dave, authentication modules need more
than username and password sometimes.  This code should make that possible.

Comment 48

[Bradley Baetz (:bbaetz)]

OK, I think I need to put my foot down here :)

Yes, justdave's stuff would probably be more flexable. But that should come
under another bug. Its more complicated (You need to update the various hidden
fields template users too, so that the passcode doesn't get sent along through
the various confirmation pages, or ideally move that set to some central
location, or hard code it into hidden-fields.html.tmpl, or whatever), and
without an impl of secureid auth, its also basically untestable. Also, the
module _cannot_ be given access to 'all the fields on the form' - I've been very
careful to split up the CGI side of things from the auth side, to allow for
email/http auth.

This bug is about moving the authentication stuff out of globals.pl and CGI.pl
into a module so that I can get on with mod_perl'ing bugzilla. Thats it.
Anything else (including the separation of CGI from the auth method, the
i18n'ing, and the limited 'make it easier to add new auth schems'-ign which I
did, and justdave's Net::LDAPing - basically everything beyond a cut and paste
into a new file) is a large bonus.

Its now been over a month since I attached the patch which got written over half
a weekend. Can we get this one in, and then someone (ie the people at Zippy
whose own secureid-login stuff uses one field in other apps) can tweak it to
work with a new scheme?

I'll look through myk's comments in a bit.

Comment 49

[Dave Miller [:justdave]]

Zippy's securID auth is already working, hacked into the existing auth mechanisms.

They won't spend money to have us completely do it over again, which we'd have
to do if this patch lands as is.  Which more or less means if this patch lands,
we can no longer sync Zippy's tree with Bugzilla, which would be a bad thing for
both of us.  If the auth module had access to any field with a given prefix on
the form, we could copy/paste the existing SecurID code to the new file with
very little modification and have it still work.

It's not as hard as you think to screen those fields from the hidden-fields
template.  Instead of passing /^Bugzilla_(login|password)$/ we just pass it
/^Bugzilla_/ and nuke anything with that prefix.  Problem solved.  The auth
forms can then be defined that any field with a name beginning with Bugzilla_
gets passed to the auth module.  We could even be nice and hack the "Bugzilla_"
off the front of each of them before passing them in.  Which would perfectly
allow for named fields to be passed in from email or whatever else you have, too.

Comment 50

[Bradley Baetz (:bbaetz)]

> Wouldn't it make more sense to return undefined if the user wasn't found?  Or is 
> zero what existing code currently expects?

Yes to both questions. This will return a Bugzilla::User object, or undef,
evetually (thats next on my list), but I decided to do this in stages.

>>+Set-Cookie: Bugzilla_logincookie= ; path=$cookiepath; expires=Sun, 30-Jun-80
00:00:00 GMT
>>+
>>+";
>Nit: this would be better as single-line strings to preserve indentation, i.e.:

Making this happen via CGI.pm is on my list too. I'll fix the newlines too,
because I suspect that this may have issues on \r\n systems :)

> For that matter, why two line breaks at the end?  Doesn't ThrowUserError still
> need to print out the Content-Type header?

Good catch. This is broken in CGI.pl, too - I just copied that bit.

> If you are going to unconditionally pass $netaddr to the database handler
> wouldn't it be good form to unconditionally trick_taint it?  This would also
> make your code for adding it to the query string much simpler, i.e.: .
> ($netaddr ? " OR logincookies.ipaddr=?" : "") . ")";

Hmm. True. I was trying to avoid the overhead, mind you. I don't think its worth
it though, so I'll change that. I do want to recheck the assertion I made the
gerv earlier (that noone will care about the extra argument). If I have to
rewrite that bit, I'll see how it turns out.

> After upgrading, will everyone have to log in all over again?

Yes. I don't think its something which we want to have compat code for - its not
worth it.

> We should replace quietly_check_login and confirm_login with the appropriate
> calls to Bugzilla::login.

Yes. There are a couple of cleanups (like reoving the ConnectToDatabase calls)
which I want to do soonish.

Comment 51

[Bradley Baetz (:bbaetz)]

Well, you do have access - Bugzilla->cgi->param('whatever') (or even
$::FORM{'whatever'}). Given that you can't use secureid auth for email because
of the time-limitation on the code, and you also can't use it for http (which
also only has a username/password fields), I don't see a problem with that.

And you've already done the changes to hidden-fields, right? :) I'd want a
different form prefix though. We don't need the Bugzilla_ since its sort of
obvious what we are (why was that there, btw?). Auth_*, perhaps?

Its not too hard to make multiple fields generic, I guess - you just have a
function returning an array of names, and then have the template do mappings for
the description, and then expose the Auth.pm interface in a TT plugin.

But my point is that I haven't actually broken anything. You had to modify the
templates anyway; so you still have to do that now. Or am I missing something?

Comment 52

[Dave Miller [:justdave]]

OK, good point.  I didn't realize the CGI object was available to us at that
point.  I guess the SecurID stuff could be shoved in without too much difficulty
then.

Well, let's take care of Myk's review comments at least. :)

Comment 53

[Bradley Baetz (:bbaetz)]

> How could useLDAP exist if you've removed it?

In old prefs files - this is the compat code (which really doens't belong there,
but thats a separate issue)

Oh, and that disabled stuff works on mainline because it prints a Content-Type
header. I got rid of that after gerv noticed it, but not the blank line. It does
print an extra C-T header currently, though.

Let me think about the multiple-fields stuff overnight; I'll attach a new
version tomorrow.

Comment 54

[Bradley Baetz (:bbaetz)]

Attachment: v6

OK, with the other fixes, but without the login-screen-genericisation code.
Thats really a separate bug, and I've outlined how to fix it. The calling of
the auth interface is localaised to Bugzilla::Auth::CGI, so if we do change
that interface to pass a hash (or whatever) then its really easy to change. In
the meantime, zippy can use Bugzilla->cgi->param to get its data.

Lets get this in now :)

Comment 55

[Gervase Markham [:gerv]]

> Index: defparams.pl
> ===================================================================
> +sub check_loginmethod {
> +    # doeditparams traverses the list of params, and for each one it checks,
> +    # then updates. This means that if one param checker wants to look at 
> +    # other params, it must be below that other one. So you can't have two 
> +    # params mutually dependant on each other.
> +    # This means that if someone clears the LDAP config params after setting
> +    # the login method as LDAP, we won't notice, but all logins will fail.
> +    # So don't do that.

What can we do to make sure no-one does this? Even a JS check on the page would
be better than nothing.

> +  {
> +   name => 'loginmethod',
> +   desc => 'The type of login authentication to use',
> +   type => 's',
> +   choices => [ 'DB', 'LDAP' ],
> +   default => 'DB',
> +   checker => \&check_loginmethod

if you aren't going to make the names more clear, please at least add a bulleted
list to the description which says what the two options mean (like e.g. the dot
description.)

> Index: editusers.cgi
> ===================================================================
> @@ -110,8 +110,8 @@
>      if ($editall) {
>          print "</TR><TR>\n";
>          print "  <TH ALIGN=\"right\">Password:</TH>\n";
> -        if(Param('useLDAP')) {
> -          print "  <TD><FONT COLOR=RED>This site is using LDAP for
authentication!</FONT></TD>\n";
> +        if(!Bugzilla::Auth->can_edit) {
> +          print "  <TD><FONT COLOR=RED>This site's authentication method does
not allow password changes through Bugzilla!</FONT></TD>\n";
>          } else {
>            print qq|
>              <TD><INPUT TYPE="PASSWORD" SIZE="16" MAXLENGTH="16"
NAME="password" VALUE=""><br>
> @@ -391,9 +391,8 @@
>          exit;
>      }
>  
> -    if(Param('useLDAP')) {
> -      print "This site is using LDAP for authentication.  To add a new user, ";
> -      print "please contact the LDAP administrators.";
> +    if(!Bugzilla::Auth->can_edit) {
> +      print "The authentication mechanism you are using does not permit
accounts to be created from Bugzilla";
>        PutTrailer();
>        exit;
>      }
> @@ -429,9 +428,8 @@
>          exit;
>      }
>  
> -    if(Param('useLDAP')) {
> -      print "This site is using LDAP for authentication.  To add a new user, ";
> -      print "please contact the LDAP administrators.";
> +    unless (Bugzilla::Auth->can_edit) {
> +      print "This site's authentication mechanism does not allow new users to
be added.";

Can we please have consistent use of either "unless" or "if (!"?

> Index: Bugzilla/Auth.pm
> ===================================================================
> +All tags B<MUST> start with the name of the module, followed by an 
> +underscore. This is to avoid conflicting tags between the various 
> +authentication modules.

Why is this particularly necessary? Some modules may want to throw common
errors. If it really is necessary, it should be "the name of the module in
lowercase" :-)

> Index: Bugzilla/Config.pm
> ===================================================================
> +    # Modularise auth code
> +    if (exists $param{'useLDAP'} && !exists $param{'loginmethod'}) {
> +        $param{'loginmethod'} = $param{'useLDAP'} ? "LDAP" : "DB";
> +    }
> +

Isn't this sort of migration done in checksetup.pl? Or not any more?

> Index: Bugzilla/Auth/CGI.pm
> ===================================================================
> +        # The IP address is valid, at least for comparing with itsself in a
> +        # subsequent login

"itself".

> +        my $template = Bugzilla->template;
> +        $template->process("account/auth/login.html.tmpl",
> +                           { 'target' => $cgi->url(-relative=>1),
> +                             'form' => \%::FORM,
> +                             'mform' => \%::MFORM,
> +                             'caneditaccount' => Bugzilla::Auth->can_edit,
> +                           }
> +                          )

I'm sure we don't format inline hashes like this; have I made this point before? :-)

> +    # The username/password may be wrong
> +    # Don't let the user know whether the username exists or whether
> +    # the password was just wrong. (This makes it harder for a cracker
> +    # to find account names by brute force)
> +    if ($authres == AUTH_LOGINFAILED) {
> +        &::ThrowUserError("invalid_username_or_password");
> +    }
> +
> +    # The account may be disabled
> +    if ($authres == AUTH_DISABLED) {

Shouldn't these all be elsifs, for clarity?

> +    my $LDAPport = "389";  #default LDAP port

Nit: missing space after comment char.

> +    return (AUTH_LOGINFAILED, undef, "LDAP_lookup_failure")

Like template names, tags are lower case by convention. You may not think it's a
big thing, but I find it visually jarring.

> Index: template/en/default/account/auth/LDAP-error.html.tmpl
> ===================================================================

We don't user upper-case in template filenames anywhere else - can we please
call this ldap-error.html.tmpl?

> +[% SWITCH auth_err_tag %]
> +  [% CASE "cannot_retreive_attr" %]

Hmm. I'm sure there's some reason the other error templates don't use SWITCH and
CASE, but I can't think of it. Are they only recently supported in TT?

> Index: template/en/default/global/code-error.html.tmpl
> ===================================================================
> +  [% ELSIF error == "auth_err" %]
> +    [% title = "Internal Authentication Error" %]
> +    [%# Authentication errors are in a template depending on the auth method,
> +        for plugability.
> +      #%] 
> +    [% INCLUDE "account/auth/$authmethod-error.html.tmpl" %]

2 gs in pluggability. You are certain $authmethod is not user-changeable (so we
won't have security problems)?

Gerv

Comment 56

[Bradley Baetz (:bbaetz)]

> What can we do to make sure no-one does this? Even a JS check on the page would
> be better than nothing.

Nothing. editparams isn't set up for anything more sophisticated.

> if you aren't going to make the names more clear, please at least add a
> bulletedlist to the description which says what the two options mean (like e.g.
> the dot description.)

Done - I used <dl> rather than <ul> though.

> Can we please have consistent use of either "unless" or "if (!"?

Done

> Why is this particularly necessary? Some modules may want to throw common
> errors. If it really is necessary, it should be "the name of the module in
> lowercase" :-)

Um, its not since I redid it - you can call them whatever you want, as long as
theres a tag in the appropriate account/auth/foo-error.html.tmpl template :) I
removed that paragraph.

> Isn't this sort of migration done in checksetup.pl? Or not any more?

Yes; that code is called from checksetup. It was moved there because of how the
param reading was done; it may be possible to move it back into checksetup now.

> I'm sure we don't format inline hashes like this; have I made this point
> before? :-)

Yes, and I disagreed with you :) There aren't really that many other examples of
this nested stuff (where the hashref isn't the only param) to copy

> Shouldn't these all be elsifs, for clarity?

Not really. Each block exits in some way, either via |exit| or throw.

> Like template names, tags are lower case by convention

Oops. s/LDAP_//.

> We don't user upper-case in template filenames anywhere else - can we please
> call this ldap-error.html.tmpl?

No. It should be the same name as the module, case included.

> Hmm. I'm sure there's some reason the other error templates don't use SWITCH
> andCASE, but I can't think of it. Are they only recently supported in TT?

Nope. I don't know why the other files don't use them, but CASE is better, I think.

> You are certain $authmethod is not user-changeable (so we won't have
> security problems)?

Yes, its set by Bugzilla::Auth::CGI (we can't just take the module name, because
cookies could theoretically throw an error) It'll also die under taint mode if
the value is tainted.

Gerv: Alos, bug 196236 was not the bug you were looking for...

Comment 57

[Bradley Baetz (:bbaetz)]

I've fixed this stuff locally, but I'll wait for gerv's comments on my comments,
and justdave's review before I upload the final version.

Comment 58

[Bradley Baetz (:bbaetz)]

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

dave, comments? I've fixed gerv's bits locally

Comment 59

[Bradley Baetz (:bbaetz)]

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

myk, I fixed your issues (or commented on them). Thoughts?

Comment 60

[Gervase Markham [:gerv]]

> > Shouldn't these all be elsifs, for clarity?
> 
> Not really. Each block exits in some way, either via |exit| or throw.

If exactly one of those blocks is supposed to execute, it makes it much easier
to figure out what's going on if you use elsif. Otherwise, one has to read every
block to make sure the variable under test doesn't get changed by one of them
(which would produce 'fall-through'.)

> > We don't user upper-case in template filenames anywhere else - can we please
> > call this ldap-error.html.tmpl?
> 
> No. It should be the same name as the module, case included.

Why? Consistency in filenames is important; if we start adding upper-case ones,
my tab completion brain has to start remembering case information for filenames
as well as the names. And I'd rather not do that.

> Nope. I don't know why the other files don't use them, but CASE is better, I 
> think.

Oh, I agree :-) I was just wondering why we weren't using it elsewhere.

By the way, r=gerv. I would strongly prefer you fix the filename case issue, but
if we still disagree, I'd be happy to accept a justdave ruling.

Gerv

Comment 61

[Bradley Baetz (:bbaetz)]

Well, else-after-return is one of brendan's nits, which feeds into mozilla
review requiresments, which feeds into me. Or something. I thikn I did it
originally because the cases did fall through at one point. I still prefer it
this way, though.

I want to keep the file names matching for consistency. I'm not too convinced by
my own argument though - what do other reviewers think? (Hint, hint... :)

Comment 62

[Jacob Steenhagen]

My preference would be to keep the filenames lower case.  We can't really force
it to be case sensitive as win32 can't have a LDAP.tmpl and ldap.tmpl in the
same directory (not sure why we'd want to, but we can't :P ). Therefore, I think
we should just lcase() that param before passing it to tt.

Comment 63

[Bradley Baetz (:bbaetz)]

Attachment: v7

Comment 64

[Bradley Baetz (:bbaetz)]

Attachment: v7

Comment 65

[Bradley Baetz (:bbaetz)]

Comment on attachment 118088 [details] [diff] [review]
v7

Oops, wrong version...

Comment 66

[Bradley Baetz (:bbaetz)]

Comment on attachment 118089 [details] [diff] [review]
v7

gerv said r=gerv already

Comment 67

[Bradley Baetz (:bbaetz)]

Checked in!

Comment 68

[Dave Miller [:justdave]]

Just for the record, in reference to comment 62, Mac OS X has the same problem
when using the HFS+ filesystem.

Comment 69

[Bradley Baetz (:bbaetz)]

Yeah, but it wouldn't have been an issue then, since the os would do the folding
from LDAP to ldap for us. I don't believe that there are any case-insensitive
non-case-folding filesystems arround.