Closed
Bug 173761
Opened 22 years ago
Closed 22 years ago
Need ability to always require login
Categories
(Bugzilla :: User Accounts, enhancement, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: mozilla, Assigned: myk)
Details
Attachments
(3 files, 2 obsolete files)
2.25 KB,
patch
|
Details | Diff | Splinter Review | |
332 bytes,
patch
|
Details | Diff | Splinter Review | |
3.38 KB,
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
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•22 years ago
|
||
attachment 102472 [details] [diff] [review] is against 2.16.1
Comment 3•22 years ago
|
||
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•22 years ago
|
||
Good point. Here's the updated patch (again, against 2.16.1).
Attachment #102472 -
Attachment is obsolete: true
Reporter | ||
Comment 5•22 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•22 years ago
|
||
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•22 years ago
|
||
This patch is against 2.17.1 and avoids sending multiple cookies
Updated•22 years ago
|
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
Updated•22 years ago
|
Attachment #107429 -
Flags: review?(justdave)
Comment 8•22 years ago
|
||
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 9•22 years ago
|
||
Comment on attachment 107429 [details] [diff] [review] Patch against 2.17.1 see Gerv's comments. :)
Attachment #107429 -
Flags: review?(justdave) → review-
Comment 10•22 years ago
|
||
[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•22 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.
Comment 12•22 years ago
|
||
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•22 years ago
|
||
Attachment #107429 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #107556 -
Flags: review?(gerv)
Comment 14•22 years ago
|
||
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+
Comment 15•22 years ago
|
||
Don't forget that comment Gerv wants before you check it in.
Flags: approval+
Comment 16•22 years ago
|
||
Checked in with added comment
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•