Closed Bug 698423 Opened 13 years ago Closed 13 years ago

Extensible login system does not allow fatal errors part way down the stack

Categories

(Bugzilla :: User Accounts, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: gerv, Assigned: gerv)

Details

Attachments

(1 file)

Bugzilla/Auth/Login/Stack.pm has the following code:

sub get_login_info {
    my $self = shift;
    my $result;
    foreach my $object (@{$self->{_stack}}) {
        # See Bugzilla::WebService::Server::JSONRPC for where and why
        # auth_no_automatic_login is used.
        if (Bugzilla->request_cache->{auth_no_automatic_login}) {
            next if $object->is_automatic;
        }
        $result = $object->get_login_info(@_);
        $self->{successful} = $object;
        last if !$result->{failure};
        # So that if none of them succeed, it's undef.
        $self->{successful} = undef;
    }
    return $result;
}

if get_login_info on an item in the stack returns a failure code - any failure code - it merely moves on to the next item in the stack. This means that there is no way for an item not at the bottom to say "yes, I can confirm the user did try to log in using my method, and that the login failed. Please don't try any other methods, but pass the failure to the user." Only the bottom item in the stack gets to pass its failure code to the user.

Say, for example, the login stack is "BrowserID,CGI". if a user is on a resource which is LOGIN_OPTIONAL, and they go to log in, and BrowserID fails fatally, it will return AUTH_LOGINFAILED. The Stack code above will then go on to try CGI, but username and password are not defined, so CGI returns AUTH_NODATA, which is the final return value from the stack. Because login is optional for this resource, this means no error is thrown - the page just loads, but in a not-logged-in state. This is bad UX.

Items in the stack should be able to return success, pass-through or final failure.

Gerv
(In reply to Gervase Markham [:gerv] from comment #0)
> is optional for this resource, this means no error is thrown - the page just
> loads, but in a not-logged-in state. This is bad UX.

I don't see why this is bad UX. If you don't want the CGI method, don't include it in the stack. If you want an error to be thrown, the page should require a login, not an optional one.
Severity: normal → enhancement
I do want the CGI method - I want Bugzilla to support multiple methods of logging in. That's the entire point of the existence of Stack.pm - so you can stack e.g. Env authentication and CGI authentication.

However, when someone attempts to log in using the BrowserID method, I want them to be shown the results of their attempt to log in using BrowserID (AUTH_LOGINFAILED), not the results of their non-existent attempt to log in using CGI (AUTH_NODATA). That seems to make sense to me.

If I go to 
https://bugzilla.mozilla.org/index.cgi?GoAheadAndLogIn=1
and try and log in using CGI and fail, I get an error, not just the same page again. So if I go to that page and try and log in using BrowserID, I should also get an error. My code is returning the appropriate error, but Stack.pm is ignoring it.

Gerv
Agreed. AUTH_NO_SUCH_USER should pass through, but AUTH_LOGINFAILED should proceed down the stack, most likely. We just have to think of if there are any potential security implications or behavioral implications to that.
  I mean, AUTH_LOGINFAILED should immediately tell the user about the failure.
Attached patch Patch v.1Splinter Review
Small patch to only allow specific errors to trigger pass-through down the stack.

Gerv
Assignee: user-accounts → gerv
Status: NEW → ASSIGNED
Attachment #575146 - Flags: review?(mkanat)
Comment on attachment 575146 [details] [diff] [review]
Patch v.1

Review of attachment 575146 [details] [diff] [review]:
-----------------------------------------------------------------

Looks right to me. Can you confirm this with a bogus browserid account/password? r=dkl but Max should have final say.
Attachment #575146 - Flags: review+
Comment on attachment 575146 [details] [diff] [review]
Patch v.1

Review of attachment 575146 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this actually looks very sensible. Are you sure that precedence works right in that "unless" statement, though? Usually I've had to wrap things in parentheses with a trailing "if" or "unless" that has more than one term.
Attachment #575146 - Flags: review?(mkanat) → review+
Flags: approval4.2+
Flags: approval+
Target Milestone: --- → Bugzilla 4.2
I added an extra pair of parentheses, for safety and certainty.

Committing to: bzr+ssh://bzr.mozilla.org/bmo/4.0/                              
modified Bugzilla/Auth/Login/Stack.pm
Committed revision 7951.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Auth/Login/Stack.pm
Committed revision 8002.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/                         
modified Bugzilla/Auth/Login/Stack.pm
Committed revision 7954.

Gerv
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 4.2 → ---
Target Milestone: --- → Bugzilla 4.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: