226324 - Move relogin.cgi code to Bugzilla::Auth::CGI

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Christian Reis]

As per comment, move the login bits from the relogin.cgi code to
Bugzilla::Auth::CGI->logout.

Comment 1

[Christian Reis]

Attachment: kiko_v1: move logout to Bugzilla::Auth::CGI.pm

I've tried to keep the same pattern followed for Bugzilla->login(), which takes
care of the globals, and handling off the backend bits (header/database) to the
CGI authentication module.

I'm not sure that Bugzilla::Auth::CGI is the correct place for the SendSQL, but
I couldn't a better place.

Comment 2

[Gervase Markham [:gerv]]

I'm not convinced this is right; given that we have pluggable auth, are we
supposed to be referring to Bugzilla::Auth::CGI directly from Bugzilla pm?

bbaetz should really be looking at this anyway; the Auth system is his baby.

Gerv

Comment 3

[Christian Reis]

Well, we refer to Auth::CGI already (just see Bugzilla->login), and this at
least centralizes things more than they are today.

Comment 4

[Christian Reis]

Attachment: kiko_v2: use dbh instead of old crufty SqlQuote

Comment 5

[Christian Reis]

Comment on attachment 136005 [details] [diff] [review]
kiko_v2: use dbh instead of old crufty SqlQuote 

I'm going to insist that this is correct :-) 

The pluggable authentication code is in LDAP.pm and DB.pm; the Cookie and CGI
modules are used in both cases. The changes I make are to be used by all
backends regardless.

Comment 6

[Christian Reis]

Attachment: kiko_v3: use placeholders for luck!

Comment 7

[Bradley Baetz (:bbaetz)]

The ::CGI bit snn't that generic, and unilt we have another option I really
can't know what abstraction we'll need.

Comment 8

[Christian Reis]

Which means hopefully you agree with my change :)

Comment 9

[Gervase Markham [:gerv]]

I recently checked in a fix for the JS security hole which assumed that logout()
just logged you out for this request rather than logged you out permanently.
You'll need to fix that, won't you? :-)

Otherwise it looks OK, but I still think bbaetz should look at it.

Gerv

Comment 10

[Christian Reis]

I knew that was coming back, it was a matter of time!

Comment 11

[Dave Miller [:justdave]]

for the record, we have an Auth module for Zippy's bugzilla which doesn't use
the cookies...  and the existing way the plugin system works it plugged right
in, with the exception of how it determines what parts of an account you're
allowed to modify.  Since the "real" Auth module is consulted prior to
Auth::Cookie, ours returns a success or fail directly and it never falls through
to Auth::Cookie.

Comment 12

[Bradley Baetz (:bbaetz)]

The CGI thing was meant to coexist with Bugzilla::Auth::Email,
Bugzilla::Auth::IRC, and so on. justdave's thing is more a replacement for ::Db
and ::LDAP, although its sort of in the middle a bit.

Comment 13

[Christian Reis]

bbaetz: would the logincookies table be used in Net::Auth::IRC and Email? I can
place the SQL bits elsewhere, but I think it's too soon for that, since
Net::Auth::CGI contains SQL in login() too.

So apart from fixing Gerv's problem (a logout_once or something) is there
anything else I'm missing? Bradley?

Comment 14

[Christian Reis]

Attachment: kiko_v4: add logout_session

I've added a logout_session call which implements the original logout()
semantics. I've checked and only buglist.cgi and relogin.cgi used the call, so
we're safe to change this.

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 136046 [details] [diff] [review]
kiko_v4: add logout_session

logout_request seems ugly, but I don't have a better suggestion - unlogin is
uglier.

Re the hard coding of ::CGI, maybe we need to split these modules up into
separate directories for authn/authz like apache does. Or Bugzilla::Auth could
poke ISA for both types of modules, I guess.

Lets wait until we ahve a second user before doing that, though.

Comment 16

[Dave Miller [:justdave]]

hmmm....

createaccount.cgi got missed.  It nukes Bugzilla_logincookie when it should be
calling ->logout

userprefs.cgi still mentions $::COOKIE{'Bugzilla_logincookie'}, that one
probably needs something new...  it's logging out all other logins of that user
except the current one when they change their password.  ->logout_others?  The
routine in globals that it's calling should move to Auth/CGI with the rest of
the stuff probably.

Comment 17

[Christian Reis]

Attachment: kiko_v5: include createaccount.cgi cleanup

Yep, I did miss createaccount; this patch includes a cleanup to it.

I filed bug 226754 for the InvalidateLogins cleanup. I'm going to move forward
bbaetz's review since it's really a trivial change to the additional cgi; chide
me if this wasn't right.

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 136300 [details] [diff] [review]
kiko_v5: include createaccount.cgi cleanup

looks good to me

Comment 19

[Christian Reis]

Another happy customer served:

/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.9; previous revision: 1.8
done
Checking in relogin.cgi;
/cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v  <--  relogin.cgi
new revision: 1.24; previous revision: 1.23
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.240; previous revision: 1.239
done
Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/Auth/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/CGI.pm,v  <--  CGI.pm
new revision: 1.5; previous revision: 1.4

Thanks.