917669 - invalid or expired authentication tokens and cookies should throw errors, not be silently ignored

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[:glob ✱]

currently if you pass an invalid or expired cookie or token to the webservices, they are silently ignored, and you are returned an unauthenticated response.

this behaviour is different from what happens when you authenticate with explicit username and password parameters, and also counter-intuitive.

invalid tokens and cookies passed to the webservices should be treated as invalid logins and trigger an authentication error.  invalid cookies passed to the normal web pages should continue to be ignored.

Comment 1

[Frédéric Buclin]

Throwing an error on invalid login cookies is an API change, IMO. All previous releases ignored these cookies when invalid, i.e. since Bugzilla 3.0 released in 2007. You cannot guarantee that all clients will treat the error appropriately. And this would affect *all* existing WS methods, including those marked as stable.

Tokens is another story as they are new to 5.0 and so no clients use them yet.

Comment 2

[:glob ✱]

(In reply to Frédéric Buclin from comment #1)
> And this would affect *all* existing WS methods, including those marked as stable.

true, however only xmlrpc endpoint is marked as stable.

we have made compatibility breaking changes even in stable methods before - for example the datetime format was changed in 3.6 to require UTC datetimes, and that wasn't a major release.

Comment 3

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #2)
> true, however only xmlrpc endpoint is marked as stable.

But it will be affected by this change anyway.


> we have made compatibility breaking changes even in stable methods before

That's not true. The only methods marked STABLE in 3.4 were all unaffected by the timezone change. The other methods were marked as stable *after* the timezone change in 3.6.

Comment 4

[David Lawrence [:dkl]]

What if we just throw an error for tokens as I made a patch for before and retain the current behavior of cookies which I think everyone will be fine with. Sadly it would mean moving the token checking code out of Cookie.pm like I had before which starts this all over.

dkl

Comment 5

[:glob ✱]

i think it's perfectly fine to adjust the behaviour here of invalid cookies, as it brings consistency to the api.

changing STABLE interfaces _is_ allowed, again quoting the docs:

> If we ever break a STABLE interface, we'll post a big notice in the Release Notes,
> and it will only happen during a major new release.

as 5.0 is a major release, we just need to ensure the release notes get a big notes.

Comment 6

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #4)
> What if we just throw an error for tokens as I made a patch for before and
> retain the current behavior of cookies which I think everyone will be fine
> with. Sadly it would mean moving the token checking code out of Cookie.pm
> like I had before which starts this all over.
> 
> dkl

Ok. so if throwing error for cookies/tokens only occurs for API calls, then +1. Also means
I can leave most of the code where it is.

dkl

Comment 7

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #5)
> i think it's perfectly fine to adjust the behaviour here of invalid cookies,
> as it brings consistency to the api.

I don't see which consistency you are talking about. Consistency with what?


> > If we ever break a STABLE interface, we'll post a big notice in the Release Notes,
> > and it will only happen during a major new release.
> 
> as 5.0 is a major release, we just need to ensure the release notes get a
> big notes.


If we *ever* break something doesn't mean that you *have to* break it without a strong rationale. And I don't see the rationale so far. It worked fine during these last 6 years (nobody ever reported this behavior as being a problem), and you suddenly want to break this behavior with which everbody else seems to be fine. If you read the quote above as being allowed to break stable API when you want to as long as the release notes mention it, you can as well remove STABLE from all methods. An API which can change at each new major release is not stable. It should happen only in last resort (e.g. to fix security holes or due to a major architecture change).

Comment 8

[:glob ✱]

(In reply to Frédéric Buclin from comment #7)
> (In reply to Byron Jones ‹:glob› from comment #5)
> > i think it's perfectly fine to adjust the behaviour here of invalid cookies,
> > as it brings consistency to the api.
> 
> I don't see which consistency you are talking about. Consistency with what?

authorization via cookies vs authorization via a token.

both equate to the same action, and it's completely reasonable to expect an error to be thrown when authorization fails.

>  And I don't see the rationale so far.

i've provided the rationale several times on this bug.

Comment 9

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #8)
> i've provided the rationale several times on this bug.

*Several* times? Where except in comment 0? And even there, you admit that cookies should behave differently when used in WS and when used in normal web pages. That's neither consistency nor more intuitive than the current behavior. Cookies and password+username always behaved differently, both in normal web and in WS. I consider that consistency. You consider that inconsistency. We will never agree (and so doesn't worth more comments). :)

Comment 10

[Erik Bryn]

I think the path of least resistance would be to keep the existing behavior for cookies, but to change the token behavior to throw an error.

I consider passing the username/password or the token in the params an explicit action by the API consumer. They intend on making an authenticated request. Therefore, I think throwing an error when authentication fails makes sense.

Cookies, on the other hand, are implicitly passed by the browser. Because of that, I can somewhat understand why the decision was made to not throw an error.

The important thing here is for the behavior to meet user expectations. I can't imagine an argument against throwing an error if authentication fails when you're explicitly providing credentials in the form of a username/password or token.

Comment 11

[David Lawrence [:dkl]]

Attachment: 917669_1.patch

Comment 12

[:glob ✱]

(In reply to Erik Bryn from comment #10)
> Cookies, on the other hand, are implicitly passed by the browser. Because of
> that, I can somewhat understand why the decision was made to not throw an
> error.

the context we're talking about is using cookies to perform authorisation for API calls, not cookies passed implicitly by the browser during normal browsing -- in other words the cookies are being used as authorisation tokens, and should be treated as such.

Comment 13

[:glob ✱]

Comment on attachment 807443 [details] [diff] [review]
917669_1.patch

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

this looks sane; just need some changes to the i_am_webservice method.

::: Bugzilla/Auth/Login/Cookie.pm
@@ +82,4 @@
>                         WHERE cookie = ?", undef, $login_cookie);
>              return { user_id => $user_id };
>          }
> +        elsif(i_am_webservice()) {

nit: need a space after elsif

@@ +88,3 @@
>      }
>  
>      # Either the he cookie is invalid, or we got no cookie. We don't want 

please touch this comment to mention the new web-service-only behaviour.

::: Bugzilla/Util.pm
@@ +241,5 @@
> +    my $usage_mode = Bugzilla->usage_mode;
> +    my $is_webservice = $usage_mode eq USAGE_MODE_XMLRPC
> +                        || $usage_mode eq USAGE_MODE_JSON
> +                        || $usage_mode eq USAGE_MODE_REST;
> +    return $is_webservice ? 1 : 0;

USAGE_MODE_* are integers, use == instead of eq
there's no need for the $is_webservice variable, return the boolean expr directly.

could you replace the check in Bugzilla/Template.pm's txt filter with i_am_webservice.

Comment 14

[David Lawrence [:dkl]]

Attachment: 917669_2.patch

Comment 15

[:glob ✱]

Comment on attachment 809186 [details] [diff] [review]
917669_2.patch

r=glob

Comment 16

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/Template.pm
modified Bugzilla/Util.pm
modified Bugzilla/WebService.pm
modified Bugzilla/Auth/Login/Cookie.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8758.

Comment 17

[Frédéric Buclin]

Added to relnotes for 5.0rc1.