Closed Bug 226754 Opened 16 years ago Closed 16 years ago

Move InvalidateLogins into Bugzilla::Auth::CGI

Categories

(Bugzilla :: User Accounts, defect)

2.17.6
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: kiko, Assigned: kiko)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch v0 (WIP): move stuff around. (obsolete) — Splinter Review
I'm still not sure on how this should be done, just checkpointing.
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
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.
Attachment #136391 - Attachment is obsolete: true
Attachment #136630 - Flags: review?(bbaetz)
Attached patch kiko_v2: actually works (obsolete) — Splinter Review
Argh. Forgot a $class in logout_user. This one is really ready, I swear <wink>
Attachment #136630 - Attachment is obsolete: true
Attachment #136633 - Flags: review?(bbaetz)
Attachment #136630 - Flags: review?(bbaetz)
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 :)
Attachment #136633 - Flags: review?(bbaetz) → review-
Attached patch kiko_v3: your wish is my command (obsolete) — Splinter Review
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.
Attachment #136633 - Attachment is obsolete: true
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.
Attachment #136912 - Flags: review?(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").
Attachment #136912 - Flags: review?(justdave) → review-
kiko: you mumbled about this bug in the b.m.o. upgrade tracking bug. Do we still
need it for that?

Gerv
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.
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.
Attachment #136912 - Attachment is obsolete: true
Attachment #143138 - Flags: review?(justdave)
> +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.
OS: Linux → All
Hardware: PC → All
Blocks: 225818
Fixed vlad's comment locally, thanks -- it will be in the version checked in
(and any patches that ensue from here).
Target Milestone: --- → Bugzilla 2.18
Attachment #143138 - Flags: review?(bugreport)
Blocks: 236678
Comment on attachment 143138 [details] [diff] [review]
kiko_v4: comments addressed, removed access to Bugzilla_logincookie, improve docs

bitrotted already
Attachment #143138 - Flags: review?(justdave)
Attachment #143138 - Flags: review?(bugreport)
Attached patch kiko_v5: unrotSplinter Review
Removed the (minor, in-comment) conflict. Reviews much appreciated!
Attachment #143138 - Attachment is obsolete: true
Attachment #144785 - Flags: review?(bugreport)
Attachment #144785 - Flags: review?(justdave)
Comment on attachment 144785 [details] [diff] [review]
kiko_v5: unrot

looks good to me, but I would like a 2xr on this.
Attachment #144785 - Flags: review?(bugreport) → review+
Attachment #144785 - Flags: review?
Attachment #144785 - Flags: review?(bbaetz)
Attachment #144785 - Flags: review?
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
Attachment #144785 - Flags: review?(justdave)
Attachment #144785 - Flags: review?(bbaetz)
Flags: approval?
Flags: approval? → approval+
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 # :-( )
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.