226754 - Move InvalidateLogins into Bugzilla::Auth::CGI

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Christian Reis]

Continuing the Auth cleanup I did in bug 226324, justdave has recommended moving
InvalidateLogins into Bugzilla::Auth::CGI. It's currently used in the following
files:

globals.pl
token.cgi
userprefs.cgi
editusers.cgi

Should be fairly straightforward.

Comment 1

[Christian Reis]

Attachment: v0 (WIP): move stuff around.

I'm still not sure on how this should be done, just checkpointing.

Comment 2

[Bradley Baetz (:bbaetz)]

Comment on attachment 136391 [details] [diff] [review]
v0 (WIP): move stuff around.

qw() rather than qw{} is the prefered style.

Also, you  need to Bugzilla->logout_user, since we're calling this as a class

Comment 3

[Christian Reis]

Attachment: kiko_v1: refactor logout()s, add logout_by_id()

Okay, this is ready do go. I ended up not adding the logout_user API to
Bugzilla::Auth::CGI, splitting the backend function into logout() and
clear_cookies(). The Bugzilla.pm module got two new functions -- logout_user
and logout_by_id, the former currently only being used by the latter but which
should be advertised as the "right" way to logout another user (when the
modularization is done).

The change to createaccount.cgi makes it actually log out the user -- it was
actually broken since we never logged the user in!

Note that I seem to have regressed erasing from the logincookies table in bug
bug 226324 -- Bugzilla->logout() didn't supply a user argument to
Bugzilla::Auth::CGI->logout()! This is fixed in this patch too, and the way
this code is organized makes it quite impossible to regress that again. Sorry.

Comment 4

[Christian Reis]

Attachment: kiko_v2: actually works

Argh. Forgot a $class in logout_user. This one is really ready, I swear <wink>

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 136633 [details] [diff] [review]
kiko_v2: actually works

My personal opinion, Bugzilla.pm should have no concept of what a
Bugzilla_logincookie is.  That should be handled exclusively by
Bugzilla::Auth::CGI.

>Index: Bugzilla.pm
>@@ -196,7 +218,6 @@ __END__
>-=head1 SYNOPSIS

Is there a specific reason we're removing this?

>Index: createaccount.cgi
>@@ -38,12 +38,12 @@ use vars qw(
>+# Verify login here so that logout() works as expected, and so the
>+# correct footer is displayed for the error message below.
>+quietly_check_login();

quietly_check_login is a deprecated pass-through function.
As long as we're moving this around, why don't we use the new way and do
Bugzilla->login(LOGIN_NORMAL); ?

>Index: Bugzilla/Auth/CGI.pm
>@@ -177,21 +177,34 @@ sub login {
>+    my ($class, $user, $cookie, $keep_cookie) = @_;

As mentioned above, we shouldn't be passing $cookie in here,
Bugzilla::Auth::CGI should be able to figure that out on its own.  Instead, why
don't we make a single $which_session parameter which uses Bugzilla::Constants
for LOGOUT_ALL, LOGOUT_KEEP_CURRENT, and LOGOUT_CURRENT_ONLY.  (Feel free to
make up better names for the constants :)

Comment 6

[Christian Reis]

Attachment: kiko_v3: your wish is my command

Thanks a lot for the comments, justdave. It's the sort of thing that improves a
patch so much it makes everything worthwhile :-)

I've rearranged functions in Bugzilla.pm and CGI.pm. CGI.pm in particular now
has an invalidate_logins, which kills logins for a specified user. The API is
also used internally from CGI->logout(), and the final result is actually quite
clean. Bugzilla.pm now calls invalidate_logins from the logout_user_*
functions, leaving cookies alone (as it very well should!)

I've added API documentation to Auth.pm that describes the way we want to take
this. Yes, there are XXXs, but I promise to fix those as soon as I get some
sleep and a stamp of approval on the design.

Comment 7

[Christian Reis]

Comment on attachment 136912 [details] [diff] [review]
kiko_v3: your wish is my command

If you can take a peek at this to see if it's according to The Plan, I'd
appreciate it.

This is tested and it works for all the cases we touch.

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 136912 [details] [diff] [review]
kiko_v3: your wish is my command

>Index: Bugzilla/Auth.pm

>+The source modules (currently, only
>+L<Bugzilla::Auth::CGI|Bugzilla::Auth::CGI> then use those methods to do
>+the authentication.

nit: you're missing a close paren here.  (It was missing before, but as long as
we're reformatting it we should fix it)

>@@ -96,16 +99,16 @@
> =item C<Bugzilla::Auth::get_netaddr($ipaddr)>
> 
> Given an ip address, this returns the associated network address, using
>-C<Param('loginnetmask')> at the netmask. This can be used to obtain data in
>-order to restrict weak authentication methods (such as cookies) to only some
>-addresses.
>+C<Param('loginnetmask')> at the netmask. This can be used to obtain data
>+in order to restrict weak authentication methods (such as cookies) to
>+only some addresses.

s/at the netmask/as the netmask/ ?

>Index: Bugzilla/Auth/CGI.pm

>@@ -30,6 +30,8 @@
> 
> use strict;
> 
>+use vars (%::MFORM, %::FORM);
>+

Uhh... why?  Nothing you're adding uses these.

>@@ -177,21 +179,58 @@ sub login {
> 
> }
> 
>+# XXX comment
> sub logout {
>-    my ($class, $user) = @_;
>+    my ($class, $user, $option) = @_;
>+    my $cookie = $::COOKIE{"Bugzilla_logincookie"};

Uh, no.  Use $cgi->cookie("Bugzilla_logincookie"), or if $cgi isn't available,
Bugzilla->cgi->cookie("Bugzilla_logincookie").

Comment 9

[Gervase Markham [:gerv]]

kiko: you mumbled about this bug in the b.m.o. upgrade tracking bug. Do we still
need it for that?

Gerv

Comment 10

[Christian Reis]

I guess not -- I'm a bit too busy to do it this week. I'll try and fit it in
next week if I'm lucky.

Comment 11

[Christian Reis]

Attachment: kiko_v4: comments addressed, removed access to Bugzilla_logincookie, improve docs

Includes all suggestions made. I managed to remove all the code that accessed
Bugzilla_logincookie and managed to remove that instance of %::COOKIE access. 

I managed to factor the failing part of login into logout_request; they were
identical.

Removing browser cookies is decided inside Bugzilla->login; that way I can
avoid an extra function that takes care of invalidations.

There's some documentation cleanups, but mostly reformatting and killing typos.

Comment 12

[Vlad Dascalu]

> +an incorrect login will still trigger an error, even though the lack of
> +a login 
> will be OK.

Not sure if the spacing should be changed there to put the last 2 rows on only 1
row.

Comment 13

[Christian Reis]

Fixed vlad's comment locally, thanks -- it will be in the version checked in
(and any patches that ensue from here).

Comment 14

[Joel Peshkin]

Comment on attachment 143138 [details] [diff] [review]
kiko_v4: comments addressed, removed access to Bugzilla_logincookie, improve docs

bitrotted already

Comment 15

[Christian Reis]

Attachment: kiko_v5: unrot

Removed the (minor, in-comment) conflict. Reviews much appreciated!

Comment 16

[Joel Peshkin]

Comment on attachment 144785 [details] [diff] [review]
kiko_v5: unrot

looks good to me, but I would like a 2xr on this.

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 144785 [details] [diff] [review]
kiko_v5: unrot

If you've tested, and looked in the DB to see that the appropriate bits are
deleted, r=bbaetz

Comment 18

[Christian Reis]

Cool. Checked in. (Note that for some reason, this checkin was a bit broken and
happened in two bits -- the first with a wrong bug # :-( )