241900 - Allow Bugzilla::Auth to have multiple login and validation styles

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P2)

Description

[Erik Stambaugh]

Two problems with Auth.

First, it needs to make a distinction between methods that provide user
information (login, passwd, etc.) versus those that validate that information. 
Bugzilla::Auth::CGI versus Bugzilla::Auth::DB, for example.

Second, these methods should be selectable as sequences.  That would allow a
fallback to regular DB password logins if LDAP fails.  It will become more
necessary when additional methods of providing user information are introduced, too.

I already have a patch ready that does these things, and will attach it shortly.

Comment 1

[Erik Stambaugh]

Attachment: Breaks up Auth and adds multiple-choice params

1. Breaks up the modules in Bugzilla/Auth by function.	Those modules that
cover entering user login data (eg., Auth/CGI.pm) go into Bugzilla/Auth/Login,
and those that authenticate that data (eg., Auth/DB.pm) go into
Bugzilla/Auth/Verify

2. Breaks Param('loginmethod') into user_info_method and user_verify_method,
which cover the above separation.  These params are also comma-seperated lists,
allowing more than one method to be used in sequence, if needed.  Future
versions of editparams promise to allow these to be proper arrays, and the
comma-seperated list is something of a placeholder waiting for that.

Comment 2

[Erik Stambaugh]

One side-effect of this patch is that editusers.cgi cannot tell reliably if
users were logged in via one method or another, so its ability to stop the
administrator from editing a user's attributes was taken out.

The reason I consider this acceptable is that anything external that populates
those attributes should really update them when there are differences between
the two.

can_edit becomes rather complex if you have more than one type of
authentication.  I changed it into a hash and made it so the value gets
populated with either whatever the current user successfully logged in through,
or the first one that allows new users to be added.

Comment 3

[Erik Stambaugh]

I hit submit a little early on that last one.

The can_edit hash containd keys for each modifiable attribute in the profiles
table, and one called 'add', which is true if new users may be added.

Comment 4

[Joel Peshkin]

Comment on attachment 147179 [details] [diff] [review]
Breaks up Auth and adds multiple-choice params


A few initial problems...

1) editusers no longer compiles...

2) fix indents when you remove an enclosing conditional.

Comment 5

[Erik Stambaugh]

Attachment: multiple-choice Auth

This one should address the above problems.

I'm kind of confused as to how that curly-brace mishap got through, as well as
the indentation.  Oh well.  Here's another crack at it.

Comment 6

[Erik Stambaugh]

Comment on attachment 149229 [details] [diff] [review]
multiple-choice Auth

Force of (recent) habit-- I gave the patch the wrong name.

Comment 7

[Joel Peshkin]

Comment on attachment 149229 [details] [diff] [review]
multiple-choice Auth


>-if (Param('loginmethod') eq 'LDAP') {
>+for my $verifymethod (split /,\s*/, Param('user_verify_method')) {
>+    if ($verifymethod eq 'LDAP') {
>     my $netLDAP = have_vers("Net::LDAP", 0);
>     if (!$netLDAP && !$silent) {
>         print "If you wish to use LDAP authentication, then you must install Net::LDAP\n\n";
>     }
>+    }
> }

Indents messed up here... 

Anyway, fix the indents and this will be r=joel
We should try developers and n.p.m.webtools to see if someone using LDAP cares
to test.

Comment 8

[Erik Stambaugh]

Attachment: multiple-choice Auth

This should fix all of the indentation problems in the previous patch.	I was
using the wrong cvs diff args on all of my patches.

Comment 9

[Joel Peshkin]

Comment on attachment 149868 [details] [diff] [review]
multiple-choice Auth

r=joel

Comment 10

[Dave Miller [:justdave]]

In case you're wondering, I'm just a *little* nervous about landing this prior
to 2.18, so I'm holding approval until we branch at this point.  From the sound
of things, hopefully the wait won't be much longer.

Comment 11

[Dave Miller [:justdave]]

/me opens the can of worms and dumps it on the floor

Comment 12

[Joel Peshkin]

checked in

Comment 13

[Joel Peshkin]

Doh!

This breaks password resets and confuses me now.

Backed out and reopened.

Clean up on Monday.

Comment 14

[Joel Peshkin]

The problem symptom is that the reset password link no longer appears on the
login page.  However, this is extremely confusing (I don;t know how I followed
it before).  Let's clean up the mechanism by which calls to the bottom-level
modules are directed to them, probaly by creating some decent objects.

Comment 15

[Christian Reis]

Attachment: kiko_fixup_v1 a.k.a. "prozac for #mozwebtools"

FWIW, here's a patch that fixes the problem here. I can't admit I *really*
follow the code here, nor do I know if using objects would help us (it seems
that all of the entities at hand are singletons, so classes might suffice).
Some notes on the patch:

  - Bugzilla::Auth is called as a class though its first parameter to
authenticate() was called self
  - I couldn't find a way to use Bugzilla::Auth::Verify::DB::can_edit as a
hash, so I changed it to be a method and use a hash. Could it be that the
can_edit variable is private to the package?
  - there was some funky indenting leftover, fixed.

The codepath isn't *too* confusing once you realize the main call pattern:

Bugzilla->login
  Bugzilla::Auth::Login::CGI (imported dynamically)-> login()
    Bugzilla::Auth -> authenticate()
      Bugzilla::Auth::Verify::DB (imported dynamically) -> authenticate()
	Bugzilla::Auth::Verify::DB -> can_edit() 
    Bugzilla::Auth -> can_edit()
      Bugzilla::Auth::Verify::DB -> can_edit() (via current_verify_method)

This is for the usecase that was broken and that is fixed by this patch --
there are obviously other equally clear paths that together form the picture,
however complicated it turns out at the end. I'll remind you it was not my
decision to code this in Perl <wink>.

Comment 16

[Jouni Heikniemi]

(In reply to comment #13)
> Backed out and reopened.
> Clean up on Monday.

May I suggest getting a 2nd review prior to the next checkin; you and erik are
working so close together that it's easy for you to miss things that might
concern others :-) (talking from my personal experience)

Comment 17

[Christian Reis]

That wasn't quite correct; this one is:

  Bugzilla->login
    Bugzilla::Auth::Login::CGI (imported dynamically)-> login()
      Bugzilla::Auth -> authenticate()
        Bugzilla::Auth::Verify::DB (imported dynamically) -> authenticate()
 	Bugzilla::Auth::Verify::DB -> can_edit() 
      Bugzilla::Auth -> can_edit()
        Bugzilla::Auth::Verify::DB -> can_edit() (via current_verify_method)

Comment 18

[Jouni Heikniemi]

Comment on attachment 149868 [details] [diff] [review]
multiple-choice Auth

Flagging this patch obsolete as part of testing  another flag related issue.
Besides, it is obsolete anyway. :-)

Comment 19

[Joel Peshkin]

*** Bug 250941 has been marked as a duplicate of this bug. ***

Comment 20

[Erik Stambaugh]

Attachment: Hopefully a less broken version

This patch includes the changes from kiko's patch, plus it checks to see if DB
is one of the allowed authentication methods before it bothers with the
password change token in the first place.

Right now there is no mechanism for an externally-managed password to be
changed from within Bugzilla.  A simple way to handle it would be to have a URL
that points to a password manager or at least instructions on changing the
passwords, but that could potentially mean having one URL for each verification
method, which is kind of out of the question at this time.  Maybe when
editparams gets its revamp.

Eventually, some sort of non-DB password management mechanism will be desired,
and by that point, we'll need something more sophisticated, but I don't know if
there are any solid plans for that sort of thing right now, which is good,
because that's a messy task.

Comment 21

[Christian Reis]

Is this is a diff -b, or is there some funky indentation going into Bugzilla.pm?

Comment 22

[Joel Peshkin]

Comment on attachment 152984 [details] [diff] [review]
Hopefully a less broken version

This is much claner, but the diff -b makes the patch unapplyable without
messing up indentation.  Fix that and I'll r=joel

Comment 23

[Joel Peshkin]

Also, check the patch before you post it.... this one is missing the files removed.

Comment 24

[Erik Stambaugh]

Attachment: Hopefully again...

1. I forgot to do cvsdo remove (ugh)
2. I've been wandering around thinking diff -b meant something it did not

This is a diff -u -N, and the files that are supposed to be deleted are marked
accordingly.  No more hurrying to get a patch uploaded.  Sorry about that.

Comment 25

[Christian Reis]

Comment on attachment 153046 [details] [diff] [review]
Hopefully again...


Stuff here is MHO, but I'd like you to consider the items and comment on them
if you think I'm wrong.

>Index: Bugzilla.pm

>+# $current_login_method stores the name of the login method that succeeded.
>+my $current_login_method = undef;

[snip]

>+    my $userid;
>+    for my $method (split(/,\s*/, Param('user_info_method'))) {
>+        require "Bugzilla/Auth/Login/" . $method . ".pm";
>+        $userid = "Bugzilla::Auth::Login::$method"->login($type);
>+        if ($userid) {
>+            $current_login_method = "Bugzilla::Auth::Login::$method";
>+            last;
>+        }
>+    }

A comment about this section. How about we moved this loop into Bugzilla::Auth,
so you called here:

  Bugzilla::Auth->login($type)

and we abstracted away all these details from Bugzilla.pm? You're already
providing a current verify method there, so you could store the other global
together and centralize login stuff there.

Bugzilla.pm should be as simple as possible IMO.

>+    # $current_login_method is defined when the user's login information is
>+    # found.  If it's not defined, the user shouldn't be logged in.
>+    if ($current_login_method) {
>+        $current_login_method->logout($_user, $option);
>+        if ($option != LOGOUT_KEEP_CURRENT) {
>+                $current_login_method->clear_browser_cookies();
>+            logout_request();
>+        }

Same here -- perhaps by calling logout() on Bugzilla::Auth and then having that
be a nop if current_login_method is None. The browser cookies thing is a bit
uneasy living here, but I'd need more thought to evaluate where it should go so
it can stay for now I guess.

>+    if ($current_login_method) {
>+        $current_login_method->logout($_user, LOGOUT_ALL);
>+    }

Same here.

>+  # in the future:
>+  #
>+  # user_verify_method and user_info_method should have 

maybe mark with an XXX here so we can find this easily later?

>Index: token.cgi
>+    # check verification methods
>+    my $has_db = 0;
>+    for (split (/[\s,]+/, Param("user_verify_method"))) {
>+        if (/^DB$/) {
>+            $has_db = 1;
>+        }
>+    }

Can't you provide an API for this? I hate having code like this strung through
all out APIs, it reminds me of having to maintain them after the fact :-)

>Index: Bugzilla/Auth.pm
>+# I'm willing to accept suggestions for somewhere else to put it.
>+my $current_verify_method = undef;

Looks like as good a place as any.. and with my suggestion all stored state
goes here. Of course, we could have an Auth *object*, but that's easy to change
once we centralize stuff in this module.

>Index: Bugzilla/Auth/Login/CGI.pm
>Index: Bugzilla/Auth/Login/CGI/Cookie.pm
>Index: Bugzilla/Auth/Verify/DB.pm
>Index: Bugzilla/Auth/Verify/LDAP.pm

Can you summarize the changes for me here? Did much in the files change beyond
consequences of the path rearrangement?

Comment 26

[Christian Reis]

(In reply to comment #25)
> all out APIs, it reminds me of having to maintain them after the fact :-)

all our CGIs. Sorry, was distracted by the mozilla.org reboots.

Comment 27

[Erik Stambaugh]

Attachment: A diff of the Auth files that were moved

* THIS IS NOT THE PATCH *

Generally, the only changes are a consequence of the files being moved around. 
There was also a change to can_edit that was necessary.  Trivial stuff, mostly.

Comment 28

[Erik Stambaugh]

Attachment: Slight update

(In reply to comment #25)

> A comment about this section. How about we moved this loop into
Bugzilla::Auth,
> so you called here:
> 
>   Bugzilla::Auth->login($type)
> 
> and we abstracted away all these details from Bugzilla.pm? You're already
> providing a current verify method there, so you could store the other global
> together and centralize login stuff there.
> 
> Bugzilla.pm should be as simple as possible IMO.

I like the idea of doing that, and started on it, but it looks to me like it
will be more of an involved process than it first seemed, due to being tied
into $_user, which is then tied to $_cleanup.  How about we tackle this one
later?

[...]
> >+	# $current_login_method is defined when the user's login information is

[...]
> Same here -- perhaps by calling logout() on Bugzilla::Auth and then having
that
> be a nop if current_login_method is None. The browser cookies thing is a bit
> uneasy living here, but I'd need more thought to evaluate where it should go
so
> it can stay for now I guess.

Moving the logout function would probably be easier, but I think I'd rather
tackle that at the same time as login gets moved, if it's all the same.

> >+  # in the future:
> >+  #
> >+  # user_verify_method and user_info_method should have 
> 
> maybe mark with an XXX here so we can find this easily later?

Done.

> >Index: token.cgi
> >+	# check verification methods
> >+	my $has_db = 0;
> >+	for (split (/[\s,]+/, Param("user_verify_method"))) {
> >+	    if (/^DB$/) {
> >+		$has_db = 1;
> >+	    }
> >+	}
> 
> Can't you provide an API for this? I hate having code like this strung
through
> all out APIs, it reminds me of having to maintain them after the fact :-)

Done.

> Looks like as good a place as any.. and with my suggestion all stored state
> goes here. Of course, we could have an Auth *object*, but that's easy to
change
> once we centralize stuff in this module.

Well, I took the request for suggestions out, at least.

> Can you summarize the changes for me here? Did much in the files change
beyond
> consequences of the path rearrangement?

(comment 27)

Comment 29

[Christian Reis]

Comment on attachment 153164 [details]
A diff of the Auth files that were moved

We changed this:

>+                             'caneditaccount' => Bugzilla::Auth->can_edit('new'),

But you still have this:

>+++ fix2/Bugzilla/Auth/Verify/LDAP.pm	Wed Jul 14 >+# can_edit is now a hash.
>+
>+my $can_edit = {
>+    'new' => 0,
>+    'userid' => 0,
>+    'login_name' => 0,
>+    'realname' => 0,
>+};
>+
> sub authenticate {
>     my ($class, $username, $passwd) = @_;
> 
>@@ -156,15 +166,13 @@
>     return (AUTH_OK, $userid);
> }
> 
>-sub can_edit { return 0; }
>-

Notice something wrong? I do :-)

Comment 30

[Christian Reis]

I've got a document to attach in a bit that can go into Bugzilla/Auth/README.

Comment 31

[Christian Reis]

Attachment: README_v1

This file attempts to describe the loose concepts of pluggable auth. I would
recomment it being included as Bugzilla/Auth/README as a source of some design
documentation.

While writing this file, I questioned the need for external knowledge of
"current_*_class", and now think that it should be centralized inside Auth and
the method calls proxied through as transparently as possible. So
Bugzilla->login should call Auth->login which should resolve what method of
auth is used and just call $method->login. Sound good?

Erik, *please* edit this text file -- it needs some cleaning up but contains at
least an initial framework of ideas.

Comment 32

[Erik Stambaugh]

Attachment: A fix and enhancement

Here's what I have in this one:

1. Fixed the glaring omission of the new can_edit for LDAP
2. Fixed an unreported similar omission in a call to can_edit from
createaccount.cgi
3. Per a conversation in #mozwebtools, created Bugzilla::Auth::Login::WWW,
moving CGI into a subdirectory and taking most of the WWW-specific code out of
Bugzilla.pm
4. Renamed the params and $current_*_method to use the term "class" instead of
"method," which should help avoid confusion
5. Added a slightly altered version of kiko's README file

Thoughts?

Comment 33

[Christian Reis]

Opinions? We's always gots some opinions! :)

Okay, here are the questions in outline form since hacking the patch to explain
is going to take years:

  - You modify token stuff to use a has_db method. My question comes: isn't this
in reality better and more flexibly referred to as can_edit("password")? Is the
problem that password changing code still lives in token.cgi? If so, I'd rather
we fixed that.. perhaps in a bug that went in before this one, or maybe use
can_edit("password") and then open a bug to move stuff into DB.pm?

    I say this because hardcoding has_db seems awkward if somebody goes tomorrow
and writes an ldap-password-changing-magic function.

  - I see you decided to go forth and rip the guts out of Bugzilla.pm! This
scary prospect actually looks quite good (now that I understand it) however I
have some comments wrt to it:

      + You left in a delete_user call which is a bit mysterious because it's
being called from WWW::CGI->logout_request. Maybe we'd be happier leaving the
logout_request ugliness in Bugzilla.pm for now until we figure out what's going
on with it.

       + I need to be told again why $current_login_class and login_class need
to live in Bugzilla.pm. I would think that the logout function wouldn't need to
check for $current_login_class -- let Auth::Login::WWW->logout worry about it.
That way all the submodule ickiness is centralized in WWW. How does that sound?

  - You do some user_verify_class grabbing in
defparams.pl:check_user_verify_class. Would part of that live happier inside
Auth? If so, perhaps file a bug to clean that up, and file another one to clean
up the XXX that's left in there (and block them with this bug).

Design-wise, it seems like we're close to a winner. Let's see about these
questions, do adjustments as necessary and I'll do a good look at the code to
push this in.

Comment 34

[Christian Reis]

One additional question: how does editusers.cgi know it can or can't edit a user
if we have no more calls to can_edit() in the CGI? I'm missing something there,
I think?

Comment 35

[Erik Stambaugh]

(In reply to comment #33)
>   - You modify token stuff to use a has_db method. My question comes: isn't this
> in reality better and more flexibly referred to as can_edit("password")? Is the
> problem that password changing code still lives in token.cgi? If so, I'd rather
> we fixed that.. perhaps in a bug that went in before this one, or maybe use
> can_edit("password") and then open a bug to move stuff into DB.pm?

had_db was added mainly as a way to allow a couple of the password-changing
interfaces to continue to work without anyone pretending that the existing
password-changina API is acceptable.

In the near future, I can't see any of the external verify modules actually
altering passwords, and hopefully before they are actually able to, we can see
the DB password-change stuff move into Bugzilla::Auth::Verify::DB, but that
stuff is IMO too much of a tangle to get moved into this patch (I'd like to land
it this year).

>     I say this because hardcoding has_db seems awkward if somebody goes tomorrow
> and writes an ldap-password-changing-magic function.

It's totally awkward and I hate it, but it's there for the same reason that the
password changing stuff hasn't moved yet-- because it complicates the patch a
lot more.  I'm willing to open a new bug to have it all cleaned out, if that's
acceptable.

>       + You left in a delete_user call which is a bit mysterious because it's
> being called from WWW::CGI->logout_request. Maybe we'd be happier leaving the
> logout_request ugliness in Bugzilla.pm for now until we figure out what's going
> on with it.

I stared at that for a good long time.  All it is is a destructor function for
$_user, which is fine.  The other part of logout_request clears cookies, which I
find terrible (even though it's just a hack for the old code that reads
$::COOKIE).  Anyway, it's harmless in either place, and I preferred to move the
cookie portion out of Bugzilla.pm.  The end result is identical.
 
>        + I need to be told again why $current_login_class and login_class need
> to live in Bugzilla.pm. I would think that the logout function wouldn't need to
> check for $current_login_class -- let Auth::Login::WWW->logout worry about it.
> That way all the submodule ickiness is centralized in WWW. How does that sound?

It's in Bugzilla.pm because about a thousand places call have Bugzilla->login()
hard-coded, and I determined that while the WWW-specific part of Bugzilla.pm is
something that can be easily moved in this patch, the assumption that we're
using the WWW cannot, and probably is a job for some later (extensive) clean-up
work.


>   - You do some user_verify_class grabbing in
> defparams.pl:check_user_verify_class. Would part of that live happier inside
> Auth? If so, perhaps file a bug to clean that up, and file another one to clean
> up the XXX that's left in there (and block them with this bug).

defparams has param checking functions, so that's where I put it.  I'm not sure
defparams should have to call Bugzilla::Auth in order to do the same.

As far as the XXX, there are dependencies on ideas that are only being discussed
right now and are not afaik contained in bugs.  It doesn't matter much to me
whether there's a bug open or not, but I didn't open it because I couldn't
accurately show everything that blocks it.

(In reply to comment #34)
> One additional question: how does editusers.cgi know it can or can't edit a
> user if we have no more calls to can_edit() in the CGI? I'm missing something
> there, I think?

Because editusers.cgi is an administrative interface, rather than a user
interface.  In some cases, an administrator is going to want to edit certain
aspects of a user, even though the auth system has it closed off.  All users get
data that sits in the DB, regardless of which auth module was used, and that
data is all that editusers changes.

userprefs.cgi, on the other hand, needs to follow the rules more stricly,
because altering the blocked data can be dangerous at worst or ineffectual at best.

Comment 36

[Erik Stambaugh]

Attachment: more fixing

Per a discussion with kiko:

Comment 37

[Erik Stambaugh]

(In reply to comment #36)
> Created an attachment (id=153691)
> more fixing
> 
> Per a discussion with kiko:

Guh... didn't finish that thought before hitting submit.

OK, here's what happened:

  - Moved current_login_method into WWW.pm
  - logout_request was moved back to Bugzilla.pm

Comment 38

[Christian Reis]

Comment on attachment 153691 [details] [diff] [review]
more fixing

Comment:

>Index: Bugzilla.pm
>@@ -97,11 +71,10 @@
>+    # $current_login_class is defined when the user's login information is
>+    # found.  If it's not defined, the user shouldn't be logged in.
>+    if (Bugzilla::Auth::Login::WWW->login_class) {
>+        Bugzilla::Auth::Login::WWW->login_class->logout($_user, $option);
>     }
> }
> 
>@@ -109,8 +82,9 @@
>     my ($class, $user) = @_;
>     # When we're logging out another user we leave cookies alone, and
>     # therefore avoid calling logout() directly.
>-    use Bugzilla::Auth::CGI;
>-    Bugzilla::Auth::CGI->logout($user, LOGOUT_ALL);
>+    if (Bugzilla::Auth::Login::WWW->login_class) {
>+        Bugzilla::Auth::Login::WWW->login_class->logout($_user, LOGOUT_ALL);
>+    }

Implement a Bugzilla::Auth::Login::WWW->logout() method that takes care of
this, removing the need to do these ifs here in Bugzilla.pm, and we will have a
winner.

>Index: defparams.pl

Please file a bug on cleaning up this defparams.pl mess and XXXs. Post number
in a comment.

>Index: editusers.cgi

Please add a comment explaining here that we don't check for can_edit() -- with
a good rationale as to why not.

>Index: token.cgi
>+    # check verification methods
>+    unless (Bugzilla::Auth->has_db) {

Note that bug 227686 is open to remove this and centralize this
password-changing mess.

Please add a diff of the moved files for the next round so I can confirm that
they look as good as I think they do.

Comment 39

[Erik Stambaugh]

(In reply to comment #38)
> (From update of attachment 153691 [details] [diff] [review])

> Implement a Bugzilla::Auth::Login::WWW->logout() method that takes care of
> this, removing the need to do these ifs here in Bugzilla.pm, and we will have a
> winner.

I have that.  It'll be in my next patch, RSN.


> Please file a bug on cleaning up this defparams.pl mess and XXXs. Post number
> in a comment.

bug 252173

> >Index: editusers.cgi
> 
> Please add a comment explaining here that we don't check for can_edit() -- with
> a good rationale as to why not.

We've discussed this in IRC, so I'll paraphrase for everyone on the CC list:

The problem is that now that a system may have multiple ways of getting a
person's information, be it LDAP, Auth, or the hopefully-soon-to-be-posted
environment-variable auth (bug 241903).  In my case, I'll be using
environment-variable auth with a fallback to CGI and DB, in case env auth isn't
available.

What that means, then, is that a user may be able to change their password, for
example, as far as DB is concerned, but not as far as Env or LDAP is concerned.
 Right now, it checks the current method that the user logged in with, which
should be sufficient for most situations, but may need to be re-examined in the
future.

The problem is with editusers.cgi, which is an administrative interface (regular
users never touch it).  Now you're logged in as person A and altering person B.
 What I decided to do was remove all of the checks as to whether a user may be
modified, since as long as you're not altering the main key that identifies a
user (currently login_name), there won't be any effect for an LDAP user, nor
will there be one for an Env user, once that's available.

So the answer, I guess, is that there is little reason now to care about whether
you should be allowed to edit another user's attributes.  Editing your own is
another story, though, since you can shoot yourself in the foot (I'm OK with
allowing administrators to shoot their users in the feet, though).

> Please add a diff of the moved files for the next round so I can confirm that
> they look as good as I think they do. 

Actually, there have been no new changes to DB.pm or LDAP.pm, so see the
existing attachment 153164 [details] above.

The new patch will follow shortly.

Comment 40

[Erik Stambaugh]

Attachment: with logout moved

Here it is...

Comment 41

[Christian Reis]

Comment on attachment 153699 [details] [diff] [review]
with logout moved

Don't cry.

>Index: Bugzilla.pm
> sub logout_user {
>     my ($class, $user) = @_;
>     # When we're logging out another user we leave cookies alone, and
>     # therefore avoid calling logout() directly.
>-    use Bugzilla::Auth::CGI;
>-    Bugzilla::Auth::CGI->logout($user, LOGOUT_ALL);
>+    Bugzilla::Auth::Login::WWW->logout($_user, LOGOUT_ALL);

a) this comment is outdated
b) it's $user, not $_user!

Comment 42

[Erik Stambaugh]

Attachment: i am a happy flower... look at me i am a happy f-l-o-w-e-r

> a) this comment is outdated

Not entirely outdated.	I altered it to be more specific.

> b) it's $user, not $_user!

Fixed.

Comment 43

[Erik Stambaugh]

Attachment: removing obsolete function from patch

14:28 <@kiko> ____erik, you have a leftover delete_user function...

Comment 44

[Christian Reis]

Comment on attachment 153702 [details] [diff] [review]
removing obsolete function from patch

>Index: Bugzilla.pm
>@@ -97,20 +67,14 @@
>     }
>     $option = LOGOUT_CURRENT unless defined $option;
> 
>-    use Bugzilla::Auth::CGI;
>-    Bugzilla::Auth::CGI->logout($_user, $option);
>-    if ($option != LOGOUT_KEEP_CURRENT) {
>-        Bugzilla::Auth::CGI->clear_browser_cookies();
>-        logout_request();
>-    }
>+    Bugzilla::Auth::Login::WWW->logout($_user, $option);
> }
> 
> sub logout_user {
>     my ($class, $user) = @_;
>     # When we're logging out another user we leave cookies alone, and
>-    # therefore avoid calling logout() directly.
>-    use Bugzilla::Auth::CGI;
>-    Bugzilla::Auth::CGI->logout($user, LOGOUT_ALL);
>+    # therefore avoid calling Bugzilla->logout() directly.
>+    Bugzilla::Auth::Login::WWW->logout($user, LOGOUT_ALL);
> }

This patch introduces a slight bug. The bug manifests itself because this code
is very very `smart'. I know: I wrote it. This is how the bug occurs:

  - logout_user() is called via editusers.cgi.
  - Auth::Login::WWW->logout() is called, which calls
  - Auth::Login::WWW::CGI->logout() which in turn, clears browser cookies and
logs out the admin who is using editusers.cgi (see the callsite) because
logout() just does a  clear_browser_cookies if the type isn't
LOGOUT_KEEP_COOKIES.
   - Oops, we have an angry admin -- we're not supposed to touch his cookies.

Boring, right?

Alternatives:
  - Keep the if code that does the cookie clearing and user logout inside
Bugzilla.pm. I find this really nasty and confusing, but maybe just opening a
new bug for this and XXXing it is enough.

  - Provide a special constant LOGOUT_SOMEBODY_ELSE that is special-cased
inside logout() to avoid clearing cookies too.

Any others you see? I'm sorry I'm being picky, but this code has delicate
semantics and I've done changes here that required hours of testing. I'm find
with you doing the XXX thing because this has gone through many iterations and
it's an evil leftover (I propagated or created, you could say).

Comment 45

[Erik Stambaugh]

(In reply to comment #44)

>   - Auth::Login::WWW::CGI->logout() which in turn, clears browser cookies and
> logs out the admin who is using editusers.cgi (see the callsite) because
> logout() just does a  clear_browser_cookies if the type isn't
> LOGOUT_KEEP_COOKIES.
>    - Oops, we have an angry admin -- we're not supposed to touch his cookies.

WORKSFORME.

    if ($option == LOGOUT_ALL) {
            $dbh->do("DELETE FROM logincookies WHERE userid = ?",
                     undef, $user->id);
            return;
    }

The return above is called before the browser cookies get deleted.  I tested it
out and it seems to work fine.

Comment 46

[Christian Reis]

By jove, you're right! I hadn't seen that. It's a horrible horrible hack :-)
I've filed follow-up bug 252204 and blocked it here.

Comment 47

[Christian Reis]

Comment on attachment 153702 [details] [diff] [review]
removing obsolete function from patch


Looks good (/me shuts eyes)

>Index: Bugzilla/Auth/Login/WWW/CGI.pm
>+        $template->process("account/auth/login.html.tmpl",
>+                           { 'target' => $cgi->url(-relative=>1),
>+                             'form' => \%::FORM,
>+                             'mform' => \%::MFORM,
>+                             'caneditaccount' => Bugzilla::Auth->can_edit('new'),
>+                             'has_db' => Bugzilla::Auth->has_db,

Just one thing: ensure this doesn't have to be \Bugzilla::Auth->has_db (I know
regular functions do, but I'm not sure about "package/class methods".

Comment 48

[Erik Stambaugh]

(In reply to comment #47)

> >+                             'has_db' => Bugzilla::Auth->has_db,
> 
> Just one thing: ensure this doesn't have to be \Bugzilla::Auth->has_db (I know
> regular functions do, but I'm not sure about "package/class methods".

It doesn't.  I'm sending the returned true/false value, not a function reference.

Comment 49

[Joel Peshkin]

fixed again 

Comment 50

[Christian Reis]

Erik, thanks for having the patience and taking the time to take this one
through the motions -- the end product came out really nice.