Need ability to always require login

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
User Accounts
P3
enhancement
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: Matt McHenry, Assigned: myk)

Tracking

2.16.1
Bugzilla 2.18
x86
Windows 2000
Bug Flags:
approval +

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
For open-source projects, it's a plus that bugzilla allows anyone to query bugs,
etc. without logging in.  But for other projects this isn't always desirable. 
What we'd like to do is make our bugzilla visible on the internet, but only
allow people with valid accounts to access it.  (We've also disabled the 'create
new account' page.)

In other words, any time any bugzilla page is requested but a valid login cookie
is not present, the "I need a valid email address and password to continue" page
should be displayed.
(Reporter)

Comment 1

16 years ago
Created attachment 102472 [details] [diff] [review]
patch to CGI.pl & defparams.pl implementing the 'alwaysrequirelogin' parameter

Tested on our local bugzilla and seems to work okay.  But be warned that this
is one of my first attempts at tweaking bugzilla ...
(Reporter)

Comment 2

16 years ago
attachment 102472 [details] [diff] [review] is against 2.16.1
Comment on attachment 102472 [details] [diff] [review]
patch to CGI.pl & defparams.pl implementing the 'alwaysrequirelogin' parameter

I like this.  My only concern with it is that this forces login for the index
page, and most places probably don't want that.  Should it call the
confirm_quietly_check_login() from index.cgi?
(Reporter)

Comment 4

16 years ago
Created attachment 102482 [details] [diff] [review]
patch to CGI.pl, defparams.pl, & index.cgi implementing the 'alwaysrequirelogin' parameter

Good point.  Here's the updated patch (again, against 2.16.1).
Attachment #102472 - Attachment is obsolete: true
(Reporter)

Comment 5

16 years ago
I was testing this a bit more, and I found a small problem.  If I'm logged out
and enter a bug number in the find field in the footer, it takes me to a login
page (as it should).  But once I log in, the bug display page has the following
text at the top:

Set-Cookie: Bugzilla_login=mmchenry ; path=/bugzilla; expires=Sun, 30-Jun-2029
00:00:00 GMT Set-Cookie: Bugzilla_logincookie=140 ; path=/bugzilla; expires=Sun,
30-Jun-2029 00:00:00 GMT

This doesn't seem to happen with any of the other pages (tried reports.cgi,
query.cgi, enter_bug.cgi).  I'm pretty clueless as to what's specific to
show_bug.cgi that might be causing this.

The login cookie is apparently getting set, though, because any of the links I
follow from this slightly funky show_bug.cgi page all work correctly without
asking me to log in a second time.
(Reporter)

Comment 6

16 years ago
Created attachment 103873 [details] [diff] [review]
patch to token.cgi from v 2.16.1

attachment 102482 [details] [diff] [review] broken token.cgi, which allows users to change their password
when they've forgotten it.  Of course you need to be able to access that
functionality without first logging in; that's what this patch is for.	(Note
that this attachment does not include the changes in attachment 102482 [details] [diff] [review]; you
must apply both patches.)

Comment 7

16 years ago
Created attachment 107429 [details] [diff] [review]
Patch against 2.17.1


This patch is against 2.17.1 and avoids sending multiple cookies

Updated

16 years ago
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18

Updated

16 years ago
Attachment #107429 - Flags: review?(justdave)
Comment on attachment 107429 [details] [diff] [review]
Patch against 2.17.1

>-sub quietly_check_login() {
>+my $cookie_set = 0;

$login_cookie_set? We have quite a few cookies floating around...

>+sub quietly_check_login {
>+    if (!(@_) && Param('requirelogin')) {

You might switch the order of this test; it makes it more clear what the point
is. 

>-    $userid = quietly_check_login();
>+    $userid = quietly_check_login('do_not_recurse_here');

How does this part work?

>+   name => 'requirelogin',
>+   desc => 'If this option is set, all access to the system will require ' .
>+           'a login. No anonymous users will be permitted.',

"If this option is set, all access to the system beyond the front page will
require a login. No anonymous access will be permitted.".

You will need to allow anonymous access to createaccount.cgi; a usage model
which forces login but allows anyone to get an account is also a valid one, and
people who don't want that will remove that CGI.

> # Check whether or not the user is logged in and, if so, set the $::userid 
>-quietly_check_login();
>+quietly_check_login('do_not_force_login');

This is kind of contradictory to read, because people expect
quietly_check_login() to not force a login anyway; how about:
quietly_check_login('always_allow_anonymous');

Gerv
Comment on attachment 107429 [details] [diff] [review]
Patch against 2.17.1

see Gerv's comments. :)
Attachment #107429 - Flags: review?(justdave) → review-
[Side note - once I modularise authentication, this'll probably become something
along the lines of:

Bugzilla::Auth::someting({ state => FOO })

Where FOO is:

AUTH_OPTIONAL - qcl
AUTH_DEFAULT - qcl, but confirm_login with the param
AUTH_REQUIRED - confirm_login

if therese no state param, then DEFAULT is assumed. So the two that you added
the arg to would become OPTIONAL]

IOW, don't get too hung up on the interface just yet

Comment 11

16 years ago
If bbaetz's new auth stuff is coming anythime soon, we may as well leave this as
a patch and not merge it in.  In its current form, it should merge in smoothly
until someone does something rather radical to authentication or cookie handling.

Well, its coming soon, as in 'its on the mod_perl list'. I havne't started it
yet, though, because I already have 3 trees with various rewrites, all of which
are going to massivly conflict with each other, and thats my limit, I think.

I also haven't fully thought though how this will work - the alternative to my
previous comment is to always _try_ to login, and scripts which require a login
(which are the minority) can check separately. I'm not sure how that will
interact with this bug, though, wrt the "its ok anyway" stuff - I'll need to
think about that.

IOW, don't let my work stop this bug.

Comment 13

16 years ago
Created attachment 107556 [details] [diff] [review]
patch with clearer names
Attachment #107429 - Attachment is obsolete: true

Updated

16 years ago
Attachment #107556 - Flags: review?(gerv)
Comment on attachment 107556 [details] [diff] [review]
patch with clearer names

>-    $userid = quietly_check_login();
>+    $userid = quietly_check_login('do_not_recurse_here');

This is all fine, except I still don't understand what "do_not_recurse_here"
means; I thought that the parameter to quietly_check_login was about anonymous
access, not recursion.

Add a comment explaining what's going on, and r=gerv.

Gerv
Attachment #107556 - Flags: review?(gerv) → review+
Don't forget that comment Gerv wants before you check it in.
Flags: approval+

Comment 16

16 years ago
Checked in with added comment
Status: NEW → RESOLVED
Last Resolved: 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.