726696 - All authenticated WebServices methods should require username/pass, token or a valid API key for authentication

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement, P1)

Description

[Frédéric Buclin]

Bug 718319 and bug 725663 are perfect examples of how dangerous the web is. You can do remote JSON-RPC and XML-RPC calls using <form> HTML elements and using JavaScript to send data to a remote server, even if this server says it's not OK to use XmlHttpRequest remotely. We are currently relying on the Content-Type header to accept or reject calls, assuming the browser behaves accordingly to https://developer.mozilla.org/En/HTTP_access_control#Simple_requests, but we have no guarantee about that.

To protect critical WebServices methods such as those editing parameters, user accounts, or bugs, we should require a token for all methods which alter the DB. A token wouldn't be needed for read-only methods. This token would be returned by User.login, and would be valid for X minutes (we could make it valid till the caller calls User.logout, but I fear some callers would forget to log out). The token would of course be tied to the user ID, to avoid an attacker to use his own token with someone else credentials.

Comment 1

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #0)
> We are currently relying on the
> Content-Type header to accept or reject calls, assuming the browser behaves
> accordingly to
> https://developer.mozilla.org/En/HTTP_access_control#Simple_requests, but we
> have no guarantee about that.

That may be true, but if a user is using a browser which does not correctly implement access control, their browser is broken and putting them at risk. I think it would be very difficult for Bugzilla to implement full session security without relying on some browser features being implemented right. (E.g. for example, we rely on the same origin policy to avoid data leaks.) How is relying on the HTTP access control spec different to relying on the same origin policy?

> alter the DB. A token wouldn't be needed for read-only methods. This token
> would be returned by User.login, and would be valid for X minutes (we could
> make it valid till the caller calls User.logout, but I fear some callers
> would forget to log out). The token would of course be tied to the user ID,
> to avoid an attacker to use his own token with someone else credentials.

If we do this, what is the advantage of having the token time out? It certainly makes the API more difficult to use. Even if callers forget to log out, there is no additional security risk I can see.

Or are you talking about keeping the token table a sensible size by timing tokens out after, say, a week?

Gerv

Comment 2

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #1)
> If we do this, what is the advantage of having the token time out?

In case your token is leaked, it can only be used for a short time, and by the expected user only. Without the time out, your token could be reused again and again without your knowledge, in the worst case.


> Or are you talking about keeping the token table a sensible size by timing
> tokens out after, say, a week?

No, I don't want these tokens to be stored in the DB. They would be hash tokens, not session tokens.

Comment 3

[Gervase Markham [:gerv]]

But that's also true of your password, isn't it? Given that the person who called User.login has your password, why is there more risk of the token leaking than your password leaking? Why don't we just require the password to be passed on every request, like the REST interface does?

Gerv

Comment 4

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #3)
> But that's also true of your password, isn't it? Given that the person who
> called User.login has your password, why is there more risk of the token
> leaking than your password leaking? Why don't we just require the password
> to be passed on every request, like the REST interface does?
> 
> Gerv

Well I vote that we also still allow username/password to be passed instead of tokens but no longer accept cookies in any case. Which the latter is easy as we just set 'no_automatic_login' in XMLRPC.pm.

Not being a security expert, I cannot argue the virtues of either tokens or username/passwords. It does seem that they are both just as vulnerable in this case. 

dkl

Comment 5

[Gervase Markham [:gerv]]

I think we should seek advice from the infrasec team; if they don't have experience securing this sort of API, they will know who does.

mcoates: what do you or your team need to know about Bugzilla's API to help us here? 

One day we will have a RESTful API where every request is standalone. But, currently, we have both JSON-RPC and XML-RPC APIs, where there is a specific "login" method and cookies are used thereafter to track sessions. This leaves us open to possible XSRF. What are the recommended ways of avoiding that? Having to turn every call into two calls - one to get a token and one to submit it - would suck deeply.

Gerv

Comment 6

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #5)
> Having to turn every call into two calls - one to get a token and one
> to submit it - would suck deeply.

I never said to do two calls instead of a single one. You would pass your token with each call, and this token would be returned once by User.login.

Comment 7

[Gervase Markham [:gerv]]

LpSolit: that point wasn't aimed at you :-)

Gerv

Comment 8

[Frédéric Buclin]

Now that 4.2 is released, we can make this discussion public.

Comment 9

[Max Kanat-Alexander]

  Probably the right solution here is to implement OAuth2 for Bugzilla.

Comment 10

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #9)
>   Probably the right solution here is to implement OAuth2 for Bugzilla.

The author of OAuth2 himself says that the protocol is bad, see http://www.theregister.co.uk/2012/07/28/oauth_editor_quits/.

Comment 11

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 12

[Frédéric Buclin]

Per http://www.w3.org/TR/cors/#security :

"resources for which simple requests have significance other than retrieval must protect themselves from Cross-Site Request Forgery (CSRF) by requiring the inclusion of an unguessable token in the explicitly provided content of the request.
[...]
Given the difficulty of avoiding such vulnerabilities in multi-origin interactions it is recommended that, instead of using user credentials automatically attached to the request by the user agent, security tokens which specify the particular capabilities and resources authorized be passed as part of the explicit content of a request."

Comment 13

[Frédéric Buclin]

After a lengthy discussion on IRC with glob, dkl and justdave, we finally agreed to implement the following authentication model:

It is no longer allowed to rely solely on login cookies to authenticate a user. To be authenticated, a user has 3 ways to do it:

a) pass Bugzilla_login + Bugzilla_password as arguments with the method. This authentication method already exists since Bugzilla 3.6 and will still work in the future. Scripts using this way to authenticate are not affected by this proposal.

b) for embedded methods in Bugzilla pages, such as User.get({match => "foo"}) used for autocompletion, a token will be automatically generated by Bugzilla and included in the list of arguments passed to the method. This token + login cookies will be used and compared to authenticate the user. Tokens have a limited lifetime, and are suitable for web pages. As a token is now required to validate login cookies, external scripts using login cookies only to authenticate users will no longer work as they have no way to access tokens (which are generated using the site_wide_secret key stored in localconfig). Those scripts will have to use API keys mentioned below.

c) for external API calls and if the user doesn't want to pass his credentials (login + password), an API key is required. An API key is bound to a user account and a user can create several API keys so that he can use one distinct key per external application if desired (such as an external dashboard wanting to interact with a Bugzilla installation). Allowing several API keys per user will let the user revoke them on a one-by-one basis in case he has concerns with a given application, without compromizing other applications or having to update the API key in all other applications.

For c), we will need a new DB table to store the API keys. At first glance, this table would look like this:

user_id     INT3         not null
api_key     varchar(255) not null primary key
description varchar(255)
last_used   datetime     not null

description is optional and lets the user describe what a key is used for ("Dashboard key", "Application X key").


And to quote justdave:

 <justdave> I think we should go ahead and break stuff.


As said above, this implementation is going to break all external scripts relying on cookies to authenticate users, even for scripts using solely our API marked as stable. This is a major API breakage for stable branches, and the reasoning is in bug 953105.

Comment 14

[Gervase Markham [:gerv]]

I need to think about this more, but one question: is the plan to require API keys even for idempotent methods such as GET?

Bug 953105 does not allow a malicious author access to any returned data; the risk is only from methods which make a change by the mere fact of being called. Not requiring API keys for GET methods might make certain classes of API use simpler.

Gerv

Comment 15

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #14)
> I need to think about this more, but one question: is the plan to require
> API keys even for idempotent methods such as GET?

If you don't pass an API key (nor login + password), you won't be logged in. If the method doesn't require login, then it will still work as usual. But this means that all write methods will require an API key (or login + password). And even read-only methods such as User.get({match => "foo"}) will require authentication as 'match' is not allowed for logged out users.

So if your GET methods are all read-only methods which do not require authentication, then they are not affected by this implementation. But you could get a subset of the information you got till now e.g. if you call Bug.get(), as security bugs would now be excluded from the list if you didn't pass any API key.

Comment 16

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #13)
> b) for embedded methods in Bugzilla pages, such as User.get({match =>
> "foo"}) used for autocompletion, a token will be automatically generated by
> Bugzilla and included in the list of arguments passed to the method. This
> token + login cookies will be used and compared to authenticate the user.
> Tokens have a limited lifetime, and are suitable for web pages. As a token
> is now required to validate login cookies, external scripts using login
> cookies only to authenticate users will no longer work as they have no way
> to access tokens (which are generated using the site_wide_secret key stored
> in localconfig). Those scripts will have to use API keys mentioned below. 

> And to quote justdave:
> 
>  <justdave> I think we should go ahead and break stuff.

Just to clarify on what justdave said, did he mean that we can break things on the stable branches as far as turning off cookie auth for write operations or did he mean to go ahead and implement full api key support for stable branches? 

IMO, we should only do the minimum necessary on the stable branches to alleviate the security exploit we currently know about. So basically disabled cookie auth for write operations and for ones the UI uses to to update data such as comment tags, etc. we can also look for the use-once token for the page and include that with the query parameters.
All other external scripts doing write operations will need to use username/password for authentication.

Then for 4.5+, I propose we do the full api key support for all webservices which to me is more of a feature request and not exactly critical security fix.

justdave, is that what you meant above or do do all changes even on stable branches?

dkl

Comment 17

[Frédéric Buclin]

justdave: please reply to comment 16.

Comment 19

[:glob ✱]

(In reply to Frédéric Buclin from comment #15)
> So if your GET methods are all read-only methods which do not require
> authentication, then they are not affected by this implementation.

as any read-only method which operates on bugs, comments, attachments, or users will require authentication to deal with access to non-public information, perhaps it would be simpler to apply this token requirement to all methods?

Comment 20

[Dave Miller [:justdave]]

The context of that quote was:

When we implemented the initial anti-CSRF protection for forms in the web pages, it was before we had a really stable WebServices API, and not very many people were using it yet. At that time, most of the existing scripts and external applications that interacted with Bugzilla did it by POSTing to the normal form submission CGIs in Bugzilla and emulating the form posts that our web pages would do when you submitted the form.  When we implemented the anti-CSRF tokens in the web forms, it therefore broke a bunch of the existing scripts, which then had to actually read the form they were emulating first to get the token out of it before submitting, or switch to using the (new at the time) WebServices API.

This situation really isn't any different.  This is, in fact, the same exact exploit concept, it's a plain and simple CSRF.  Our existing API doesn't have any CSRF protection.  Fixing it is going to break things, just like it did last time.  The community survived before, and scripts got updated to work with the New Way.  They will recover again.  In the interest of security this breakage is a necessary evil.

Looking at the old security advisories it looks like the initial implementation of CSRF protection was implemented on trunk and current-stable only, and older branches didn't get it.  It also looks like that was for technical reasons and not trying to avoid the old branches, because the older branches didn't have the a sufficient mechanism available to generate tokens.  So I guess the precedent here is to do this on trunk and current-stable, and say it's too invasive to backport to older branches, but make people aware of the security risk.  If there are parts of it that we can backport (like maybe a preference to disable cookie auth on API requests, which the admin can turn on if they know they aren't using any scripts that will get broken by it or something - although I think this would break autocomplete) then we probably should do what we can.

Comment 21

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #19)
> as any read-only method which operates on bugs, comments, attachments, or
> users will require authentication to deal with access to non-public
> information, perhaps it would be simpler to apply this token requirement to
> all methods?

Presumably you mean "...when authentication is required"?

We don't want to require a token for non-authenticated calls. That would unnecessarily break display-only applications such as e.g. a wiki.mozilla.org integrated buglist.

Gerv

Comment 22

[Frédéric Buclin]

There is no need to *require* an API key for read-only methods. If it's passed, the read-only method will be able to access sensitive data such as bugs restricted to some bugs. If no API key is passed, then only public data will be returned. So I agree with gerv that there is no need for an API key in all cases, only for write methods and for read-only methods which require authentication.

Comment 23

[Dave Miller [:justdave]]

Retargeting to 4.4, per comment 20 (didn't catch that when I made the comment).

Comment 24

[Frédéric Buclin]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #23)
> Retargeting to 4.4, per comment 20:

"So I guess the precedent here is to do this on trunk and current-stable, and say it's too invasive to backport to older branches, but make people aware of the security risk."


That's the relevant bit. ;)

Comment 25

[David Lawrence [:dkl]]

Removing as a blocker for 4.4.3 as bug 893195 has been backported to 4.4 and provides simple token based authentication for now. We still want to do proper API key support as outlined in comment 13 but we will do this as part of 5.0 and not hold up 4.4.3 which needs to go out before this is completed.

dkl

Comment 26

[Simon Green]

So Mark coxed me into doing this bug. Authentication is one area that I don't know intimately, but it will be good to work on it to improve my knowledge.

A few questions / comments. Please speak up now if you object to my suggestions.

1) How long should an API key be? If noone objects, I'll make it 40 characters.
2) I think the primary key of the table should be id (and the api_key value be unique). This means that links in the browser will be nicer. I also recall reading somewhere (many moons ago) that some databases don't work well with long primary keys.
3) Should we also have a revoked column (type boolean) and allow users to re-enable revoked tokens? My thought here is if you accidentally revoke the wrong token and don't want to generate a new one. It also prevents an API key from ever been reused.
4) Should we e-mail the user whenever a token in generated or revoked? I think this is an important security measure since if a user's login and password was stolen, an attacker could create a token and remain undetected for a very long time.

5) For the browser based tokens, I assume their length would be three days like all other tokens. Is that correct?
6) Do we need to revoke the token once the form has been submitted? If so, what is the best way to do this? We can't delete all temp auth tokens, since the user may have opened a second page before submitting the first. I was thinking maybe putting the csrf tokens in the eventdata, or having a form element with the tokens to revoke. Neither of these are ideal (not all forms have csrf tokens, and form elements can be easily changed to not revoke the token), so am open to better options. If not, I'll go with the form element option.

Comment 27

[Simon Green]

and 

7) Should we apply this to rest calls only (i.e. allow xmlrpc and jsonrpc to still use the old Bugzilla_token or the new API method?

There are three reason for this:
 1) It eases the transition for existing scripts. I know many guys at Red Hat will be asking for my head on a stick if we broke all RPC calls the day that we release 5.0. RPC uses would then have the ability to change from rpc + Bugzilla_token to rest + api key at their leisure.
 2) RPC calls are going away in Bugzilla 5.0 + 2
 3) RPC calls in 4.4+ already have tokens which mitigate the problem.

Comment 28

[:glob ✱]

> b) for embedded methods in Bugzilla pages, such as User.get({match => "foo"}) used for autocompletion,
> a token will be automatically generated by Bugzilla and included in the list of arguments passed to
> the method.

i'm unclear how this will work with rpc calls where the parameters are driven by user input (eg. user auto-completion).  we obviously can't pre-generate the tokens.

Comment 29

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #28)
> > b) for embedded methods in Bugzilla pages, such as User.get({match => "foo"}) used for autocompletion,
> > a token will be automatically generated by Bugzilla and included in the list of arguments passed to
> > the method.
> 
> i'm unclear how this will work with rpc calls where the parameters are
> driven by user input (eg. user auto-completion).  we obviously can't
> pre-generate the tokens.

ignore me, i totally miss-read that sentence as "the token will be generated using the arguments" instead of "the token will be passed via arguments.

Comment 30

[Dave Miller [:justdave]]

So I'd like some opinions from a few people here...  from my reading of this bug, it was originally secure because it was a proposed fix to a security vulnerability, however that vulnerability was mitigated by bug 893195.  Which means the remainder of this bug is no longer closing a vulnerability, but enhancing the way we handle the situation so that it's a more robust way of handling it.

If the above statement is true, then a) this bug should no longer be secure, because the advisory on this issue went out when bug 893195 was fixed, and b) I have no issues with allowing both the tokens and the API keys to work concurrently for at least one release to allow people to transition.

I want a few people to confirm my statement above before we do either though.

Comment 31

[Simon Green]

FWIW, I agree with both points in comment 30.

Comment 32

[Gervase Markham [:gerv]]

If this is no longer solving a security problem, why are we doing it at all?

I think that before this bug is implemented, we need to condense the 30+ comments here into a design document on the wiki, giving the problem we are solving, the rationale for the change we have chosen, and listing the API-user-visible changes and what API users will need to do to update their code.

I think that the exercise will help us to get a lot more clarity about what this bug is about.

Gerv

Comment 33

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #32)
> If this is no longer solving a security problem, why are we doing it at all?

Sorry to jump in, but do you all forgot why this bug exists? To definitely fix bug 953105 and bug 953110 and all potentially related bugs, including working around bug 957403! Bug 893195 was in no way a definitive fix. It only fixed the problem reported there. But bugs 953105 and 953110 are still valid and the current Bugzilla code is still vulnerable (all versions, including trunk).

Making this bug public would give clues about what the remaining issue is. Not fixing this bug would mean leaving Bugzilla vulnerable to these attacks. So please think again before making such considerations: yes, this bug fixes a severe security issue in the Bugzilla API; and no, bug 893195 didn't fix it!

Comment 34

[Gervase Markham [:gerv]]

OK. So the answer is we shouldn't open it up, then :-) 

(I was pointing out the inconsistency of both saying it's not a security issue and still fixing the bug. I wasn't saying we shouldn't fix the bug.)

Gerv

Comment 35

[Mark Côté [:mcote]]

I don't fully understand the issue, but I will take LpSolit's word for it that this is still a security concern. :)

Comment 36

[David Lawrence [:dkl]]

(In reply to Simon Green from comment #26)
> So Mark coxed me into doing this bug. Authentication is one area that I
> don't know intimately, but it will be good to work on it to improve my
> knowledge.
> 
> A few questions / comments. Please speak up now if you object to my
> suggestions.
> 
> 1) How long should an API key be? If noone objects, I'll make it 40
> characters.

Agreed.

> 2) I think the primary key of the table should be id (and the api_key value
> be unique). This means that links in the browser will be nicer. I also
> recall reading somewhere (many moons ago) that some databases don't work
> well with long primary keys.

Agreed.

> 3) Should we also have a revoked column (type boolean) and allow users to
> re-enable revoked tokens? My thought here is if you accidentally revoke the
> wrong token and don't want to generate a new one. It also prevents an API
> key from ever been reused.

Not sure if we need this. I propose we have a interim confirmation screen and
if the user using the UI (or the app that generated the api
key for the user via webservice) added a proper summary for the api key, it should
be sufficient.

> 4) Should we e-mail the user whenever a token in generated or revoked? I
> think this is an important security measure since if a user's login and
> password was stolen, an attacker could create a token and remain undetected
> for a very long time.

Good point so agreed.
 
> 5) For the browser based tokens, I assume their length would be three days
> like all other tokens. Is that correct?
> 6) Do we need to revoke the token once the form has been submitted? If so,
> what is the best way to do this? We can't delete all temp auth tokens, since
> the user may have opened a second page before submitting the first. I was
> thinking maybe putting the csrf tokens in the eventdata, or having a form
> element with the tokens to revoke. Neither of these are ideal (not all forms
> have csrf tokens, and form elements can be easily changed to not revoke the
> token), so am open to better options. If not, I'll go with the form element
> option.

I am not sold on this as someone mentioned before that what would happen if someone
were to send a HTML dump of a show_bug.cgi page to someone else? Such as to pass
along some bug details, etc. The recipient would have the user's temporary token 
embedded in the page and be able to use it to do authenticated requests in the 
period before the token expires. 

What if were to have a small separate set of webservice methods only for the UI 
features that still allowed cookies from same origin? Can be reliably confirm
same origin or is it still possible to spoof that? We can't really throw away the
token on form submission and give new one at that time as the user may not submit
anything right away. And if we throw away other tokens for a user when they submit
then it may break other tabs that are looking at a bug report. If I am describing
the proposed implementation incorrectly, then feel free to elaborate.

I also agree with Gerv that we need a proper design page on the wiki describing
1) what problem we are fixing, 2) how the feature will be designed and how it will
work via the UI and the API and the 3) schema layout for the stored data.

I also do not agree that we need to have the old token support to work alongside the 
new API key support. We will still support the 4.4 release for a while and people
can update to the new style rather easily as it is very similar to tokens and will just require mostly a parameter name change. We could continue to accept 'token' as a backwards compat key name.

dkl

Comment 37

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #36)
> > 3) Should we also have a revoked column (type boolean) and allow users to
> > re-enable revoked tokens? My thought here is if you accidentally revoke the
> > wrong token and don't want to generate a new one. It also prevents an API
> > key from ever been reused.
> 
> Not sure if we need this. I propose we have a interim confirmation screen and
> if the user using the UI (or the app that generated the api
> key for the user via webservice) added a proper summary for the api key, it
> should
> be sufficient.


Okay. I'm still going to have a revoked column, but it won't be possible to unrevoke a key. This will ensure that an API key can be used by another person (although the changes of the same API key being issues is VERY small).

> I am not sold on this as someone mentioned before that what would happen if
> someone
> were to send a HTML dump of a show_bug.cgi page to someone else? Such as to
> pass
> along some bug details, etc. The recipient would have the user's temporary
> token 
> embedded in the page and be able to use it to do authenticated requests in
> the 
> period before the token expires. 

Since the login cookie will also be sent, we can use both the login cookie and cookie sent in the RPC to prevent this.

> I also agree with Gerv that we need a proper design page on the wiki
> describing
> 1) what problem we are fixing, 2) how the feature will be designed and how
> it will
> work via the UI and the API and the 3) schema layout for the stored data.

Can someone set up a wiki page, mopad, google docs or other such page where this can be discussed (I'd prefer mopad or google docs as it is more collaborative than a wiki page). Remember that I don't have a Mozilla LDAP account (yet!)

> I also do not agree that we need to have the old token support to work
> alongside the 
> new API key support. We will still support the 4.4 release for a while and
> people
> can update to the new style rather easily as it is very similar to tokens
> and will just require mostly a parameter name change. We could continue to
> accept 'token' as a backwards compat key name.

I totally disagree. You are right that we support Bugzilla 4.4 (and 4.2) when Bugzilla 5.0 is released, but expecting all RPC users to change all their scripts on the day that Bugzilla 5.0 is released on their installation isn't good from a user experience point of view. Having said that, I have no problem nuking all the token code, and then backporting support specifically in bugzilla.redhat.com. We learnt from the token change is Bugzilla 4.4.3 that some teams simply can't make a change as quick as that.

Comment 38

[Simon Green]

Time to fix the typos: "I'm still going to have a revoked column, but it won't be possible to unrevoke a key. This will ensure that an API key can't be used by another person (although the chances of the same API key being issued is VERY small anyway)."

Comment 39

[Gervase Markham [:gerv]]

(In reply to Simon Green from comment #37)
> Can someone set up a wiki page, mopad, google docs or other such page where
> this can be discussed (I'd prefer mopad or google docs as it is more
> collaborative than a wiki page). Remember that I don't have a Mozilla LDAP
> account (yet!)

You don't need LDAP to use our wiki :-) However, if we feel this bug needs to stay secure, then we also need a secure space for the design document. Of the available options, that sounds like Google Docs to me.

I've set up a doc and invited everyone currently on this bug.

Gerv

Comment 40

[Gervase Markham [:gerv]]

...except mcoates, as it seems there's no Google account associated with "[e-mail]", and that would mean I'd have to send an anonymous access link.

Gerv

Comment 41

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #40)
> ...except mcoates

mcoates no longer works for mozilla.

Comment 42

[Simon Green]

Attachment: bug726696-v1.patch

As I said in a comment, User auth is definitely something I'm wasn't 100% familiar with. Until now, it 'just works'™, although I've gained much more knowledge.

Comment 43

[David Lawrence [:dkl]]

Comment on attachment 8434621 [details] [diff] [review]
bug726696-v1.patch

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

Suggestion: Keep naming consistent. I prefer api_key for parameter name when using the webservices. So we should take either &api_key=jdfkjhjskfjks or &Bugzilla_api_key=jdfkjhjskfjks.

Change Bugzilla_key to Bugzilla_api_key (api_key) (only for webservices)
Change Bugzilla_apiauth to Bugzilla_api_token (token) (only for UI ajax)

Add code to REST.pm fix_credentials() to allow for 'api_key' as a shortcut to 'Bugzilla_api_key'.

::: Bugzilla/Auth/Login/APIKey.pm
@@ +28,5 @@
> +    my ($self) = @_;
> +    my $params = Bugzilla->input_params;
> +    my ($user_id, $login_cookie);
> +
> +    my $api_key_text = trim(delete $params->{'Bugzilla_key'});

Bugzilla_api_key

@@ +29,5 @@
> +    my $params = Bugzilla->input_params;
> +    my ($user_id, $login_cookie);
> +
> +    my $api_key_text = trim(delete $params->{'Bugzilla_key'});
> +    if (! i_am_webservice() || ! $api_key_text) {

nit: remove space after !

@@ +40,5 @@
> +        # The second part checks the correct capitalisation. Silly MySQL
> +        ThrowUserError("api_key_not_valid");
> +    }
> +    elsif ($api_key->revoked) {
> +        ThrowUserError('api_key_revoked');

Single error as mentioned before:

if (!$api_key || $api_key->api_key ne $api_key_text || $api_key->revoked) {
    # The second part checks the correct capitalisation. Silly MySQL
    ThrowUserError("api_key_not_valid");
}

::: Bugzilla/Auth/Login/Cookie.pm
@@ +51,5 @@
>              $user_id = $cookie->value if $cookie;
>          }
> +
> +        # If the call is for a web service, we need to check an api token is
> +        # provided. This is different from the token below

This currently breaks JSONRPC/XMLRPC using POST because of cookies being allowed for those call types.

Example: Bugzilla/WebService/Server/JSONRPC.pm line 244

@@ +53,5 @@
> +
> +        # If the call is for a web service, we need to check an api token is
> +        # provided. This is different from the token below
> +        if (i_am_webservice()) {
> +            my $apiauth_token = Bugzilla->input_params->{Bugzilla_apiauth}

Bugzilla_api_token

@@ +58,5 @@
> +                || ThrowUserError('auth_no_token');
> +            my ($token_user_id, undef, undef, $token_type)
> +                = Bugzilla::Token::GetTokenData($apiauth_token);
> +            if ($token_type ne 'apiauth' || $user_id != $token_user_id) {
> +                ThrowUserError('auth_invalid_token', { token => $apiauth_token });

Single error 'api_token_invalid' as mentioned elsewhere.

::: Bugzilla/Template.pm
@@ +999,4 @@
>                  return $cookie ? issue_hash_token(['login_request', $cookie]) : '';
>              },
>  
> +            'get_apiauth_token' => sub {

nit: get_api_token

::: Bugzilla/Token.pm
@@ +31,5 @@
>  # Public Functions
>  ################################################################################
>  
> +# Create a token used for internal API authentication
> +sub issue_apiauth_token {

nit: issue_api_token

@@ +34,5 @@
> +# Create a token used for internal API authentication
> +sub issue_apiauth_token {
> +    # Generates a random token, adds it to the tokens table, and returns
> +    # the token to the caller.
> +    return _create_token(Bugzilla->user->id, 'apiauth', '');

s/apiauth/api_token/

::: Bugzilla/User/APIKey.pm
@@ +19,5 @@
> +# Overriden Constants that are used as methods
> +#####################################################################
> +
> +use constant DB_TABLE       => 'user_api_keys';
> +use constant DB_COLUMNS     => qw(id user_id api_key description revoked last_used);

nit: one column per line

@@ +77,5 @@
> +=head1 SYNOPSIS
> +
> +  use Bugzilla::User::APIKey;
> +
> +  my $api_key = Bugzilla::User::APIKey->new($id);

my $api_key = Bugzilla::User::APIKey->new({ name => $api_key });

@@ +81,5 @@
> +  my $api_key = Bugzilla::User::APIKey->new($id);
> +
> +  # Class Functions
> +  $user_api_key = Bugzilla::User::APIKey->create({
> +      description   => $description,

nit: extra space before =>

::: Bugzilla/WebService.pm
@@ +142,4 @@
>  
>  =over
>  
> +=item C<Bugzilla_key>

Bugzilla_api_key or api_key

@@ +189,5 @@
> +
> +=item C<User.login>
> +
> +You can use L<Bugzilla::WebService::User/login> to log in as a Bugzilla
> +user. This issues a token that you must then use in future calls.

As mentioned before, I think we can leave this for API key management via webservices or change the name to something more appropriate.

::: Bugzilla/WebService/User.pm
@@ +429,5 @@
>  =head1 Logging In and Out
>  
> +These method are now deprecated, and will be removed in the release after
> +Bugzilla 5.0. The correct way of use these REST and RPC calls is noted in
> +L<Bugzilla::WebService>

I would not remove the User.login method. I would explain that it will not longer return a 'token' value pass a certain time. We can however keep it and have it return the API key for a given description or if new_key => 1 passed in then generate a new API key and return it. With optional description (new_key_description). This way we do not have to rely on using the web UI for generating keys (think mobile apps).

@@ +500,5 @@
>  =item C<restrict_login> was added in Bugzilla B<5.0>.
>  
> +=item C<token> was added in Bugzilla B<4.4.3>.
> +
> +=item This function will be removed in the release after Bugzilla 5.0, in favour of API keys.

This key will be removed.

@@ +565,5 @@
>  
>  =item Added in Bugzilla B<5.0>.
>  
> +=item This function will be removed in the release after Bugzilla 5.0, in favour of API keys.
> +

valid_login could verify an api key based on user name and password maybe? Or we could do away with it as api keys will not expire as often as tokens do unless someone explicitly revokes it.

::: js/bug.js
@@ +22,4 @@
>              method : "Bug.possible_duplicates",
>              id : YAHOO.bugzilla.dupTable.counter,
>              params : {
> +                Bugzilla_apiauth: BUGZILLA.apiauth_token,

Bugzilla_api_token: BUGZILLA.api_token

::: js/comment-tagging.js
@@ +50,5 @@
>              return YAHOO.lang.JSON.stringify({
>                  method : "Bug.search_comment_tags",
>                  id : YAHOO.bugzilla.commentTagging.counter,
> +                params : {
> +                    Bugzilla_apiauth: BUGZILLA.apiauth_token,

Bugzilla_api_token: BUGZILLA.api_token

::: js/field.js
@@ +825,4 @@
>            method : "User.get",
>            id : YAHOO.bugzilla.userAutocomplete.counter,
>            params : [ { 
> +            Bugzilla_apiauth: BUGZILLA.apiauth_token,

Bugzilla_api_token: BUGZILLA.api_token

::: template/en/default/account/prefs/apikey.html.tmpl
@@ +12,5 @@
> +  #%]
> +
> +<p>API keys are used to authenticate REST calls. You can create more than one
> +API key if required. Each API key has an optional description which can help
> +you record what each key is used for.</p>

indent two spaces

Maybe show an example of using it with REST either in the URL (/rest/bug/35?api_key=<APIKEY>) or in a PUT/POST body ({ "api_key":"<APIKEY>" }).
And/Or we can link to the WebService docs for more information.

@@ +72,5 @@
> +providing a description for the API key. The API key will be randomly
> +generated for you.</p>
> +
> +<input type="checkbox" name="new_key" id="new_key"> Generate a new API key
> +<p>Description: <input name="new_description" id="new_description"></p>

Maybe all on one line

[_] Generate a new API key with optional description [______________________]

::: template/en/default/account/prefs/prefs.html.tmpl
@@ +40,5 @@
>                link => "userprefs.cgi?tab=permissions", saveable => "0",
> +              doc_section => "using.html#permissions" },
> +            { name => "apikey", label => "API Keys",
> +              link => "userprefs.cgi?tab=apikey", saveable => "1",
> +              doc_section => "using.html#apikey" } ] %]

The docs need to be created in docs/en/rst/using.rst for this link to work

::: template/en/default/global/header.html.tmpl
@@ +166,5 @@
>                  version_required:
>                      'You must select a Version for this [% terms.bug %].'
>              }
> +            [% IF javascript_urls.containsany(['js/bug.js', 'js/field.js', 'js/comment-tagging.js']) %]
> +              , apiauth_token: '[% get_apiauth_token FILTER js FILTER html %]'

, api_token: '[% get_apiauth_token FILTER js FILTER html %]'

::: template/en/default/global/user-error.html.tmpl
@@ +103,5 @@
> +    correctly.
> +
> +  [% ELSIF error == "api_key_revoked" %]
> +    [% title = "Invalid API key" %]
> +    The API key you specified has been revoked by the user that created it.

Combine the two above to be a single api_key_invalid error. We don't want to necessarily give away that a key has been revoked.

The API key you specified is either invalid or has been revoked. Please check that your key is valid.

@@ +235,5 @@
> +
> +  [% ELSIF error == "auth_no_token" %]
> +    [% title = 'A token error occurred' %]
> +    When making internal API calls, you must specify a token with the
> +    Bugzilla_apiauth key.

These could also be combined to be api_token_invalid.

The API token [% IF token %]'[% token FILTER html %]'[% END %] is either not provided, not valid, or has expired.

::: userprefs.cgi
@@ +519,5 @@
> +    # Do it in a transaction.
> +    $dbh->bz_start_transaction;
> +
> +    # Update any existing keys
> +    my $api_keys = Bugzilla::User::APIKey->match({user_id => $user->id});

space after and before { }

Comment 44

[Simon Green]

There are some points that I strongly disagree with in this review. If it isn't mentioned below, I agree with it :)

(In reply to David Lawrence [:dkl] from comment #43)
> Review of attachment 8434621 [details] [diff] [review]:
> ::: Bugzilla/WebService/User.pm
> @@ +429,5 @@
> >  =head1 Logging In and Out
> >  
> > +These method are now deprecated, and will be removed in the release after
> > +Bugzilla 5.0. The correct way of use these REST and RPC calls is noted in
> > +L<Bugzilla::WebService>
> 
> I would not remove the User.login method. I would explain that it will not
> longer return a 'token' value pass a certain time. We can however keep it
> and have it return the API key for a given description or if new_key => 1
> passed in then generate a new API key and return it. With optional
> description (new_key_description). This way we do not have to rely on using
> the web UI for generating keys (think mobile apps).

I STRONGLY disagree with this. For a start, this wasn't part of the design doc that was created in comment 39. Even so, if we are going to allow someone to generate a key via a GET request (REST User.login is get) with a username and password, we are not going to fix the problem we are trying to solve. 

If a user doesn't want to use the web UI to generate an API key, they are free to send Bugzilla_login and Bugzilla_password with each request.

> @@ +500,5 @@
> >  =item C<restrict_login> was added in Bugzilla B<5.0>.
> >  
> > +=item C<token> was added in Bugzilla B<4.4.3>.
> > +
> > +=item This function will be removed in the release after Bugzilla 5.0, in favour of API keys.
> 
> This key will be removed.

I guess this is dependent on the above comment. The plan as per the design doc is to nuke User.login once we remove tokens authentication.
 
> @@ +565,5 @@
> >  
> >  =item Added in Bugzilla B<5.0>.
> >  
> > +=item This function will be removed in the release after Bugzilla 5.0, in favour of API keys.
> > +
> 
> valid_login could verify an api key based on user name and password maybe?
> Or we could do away with it as api keys will not expire as often as tokens
> do unless someone explicitly revokes it.

No, once tokens are gone, there will be no use of passwords in the API calls (other than by passing Bugzilla_login and Bugzilla_password)

> ::: template/en/default/account/prefs/prefs.html.tmpl
> @@ +40,5 @@
> >                link => "userprefs.cgi?tab=permissions", saveable => "0",
> > +              doc_section => "using.html#permissions" },
> > +            { name => "apikey", label => "API Keys",
> > +              link => "userprefs.cgi?tab=apikey", saveable => "1",
> > +              doc_section => "using.html#apikey" } ] %]
> 
> The docs need to be created in docs/en/rst/using.rst for this link to work

Oops.
 
> ::: template/en/default/global/user-error.html.tmpl
> @@ +103,5 @@
> > +    correctly.
> > +
> > +  [% ELSIF error == "api_key_revoked" %]
> > +    [% title = "Invalid API key" %]
> > +    The API key you specified has been revoked by the user that created it.
> 
> Combine the two above to be a single api_key_invalid error. We don't want to
> necessarily give away that a key has been revoked.
 
I disagree. It's about time we give user's helpful error messages. There is no security implications in letting a user know that a token has been revoked vs. not exists. It still won't work, and the API key by itself won't let you know who owns it.

> @@ +235,5 @@
> > +
> > +  [% ELSIF error == "auth_no_token" %]
> > +    [% title = 'A token error occurred' %]
> > +    When making internal API calls, you must specify a token with the
> > +    Bugzilla_apiauth key.
> 
> These could also be combined to be api_token_invalid.
> 
> The API token [% IF token %]'[% token FILTER html %]'[% END %] is either not
> provided, not valid, or has expired.

I think it is important to show the difference between not providing a token, and specifying and invalid / revoked token. It makes it easier for the user to understand what is wrong. Let's assume that the users writing an API call are doing so for the first time (because at some point everyone is). The more helpful we can be (without compromising security), the better.

I'm going to submit a new patch without the changes that I've objected to, for further review. If anyone disagrees with anything I've said in this comment, please let me know.

Comment 45

[David Lawrence [:dkl]]

(In reply to Simon Green from comment #44)
> I STRONGLY disagree with this. For a start, this wasn't part of the design
> doc that was created in comment 39. Even so, if we are going to allow
> someone to generate a key via a GET request (REST User.login is get) with a
> username and password, we are not going to fix the problem we are trying to
> solve. 
> 
> If a user doesn't want to use the web UI to generate an API key, they are
> free to send Bugzilla_login and Bugzilla_password with each request.

The document does mention removal of User.login/logout/valid_login to remove support for
tokens but after more thought post document, I feel we still need some methods for management 
of api keys via the webservices API. App makers or other third-party clients such as BzDeck
should not have to send someone to the Bugzilla preferences UI, have them cut and paste the
key into a form field of the app/client. I realize we send a verification email from the
UI for added security which we can still do when someone creates the api key via a webservice
call. You still need a valid username/password when create an api key via webservice so it is
no more insecure that using the web UI. We could make User.login (or name it something different)
require POST to disallow the username/password from the server logs but we already allow with GET 
for other api calls.

glob, justdave, please give your thoughts on this.

> > @@ +500,5 @@
> > >  =item C<restrict_login> was added in Bugzilla B<5.0>.
> > >  
> > > +=item C<token> was added in Bugzilla B<4.4.3>.
> > > +
> > > +=item This function will be removed in the release after Bugzilla 5.0, in favour of API keys.
> > 
> > This key will be removed.
> 
> I guess this is dependent on the above comment. The plan as per the design
> doc is to nuke User.login once we remove tokens authentication.

I am fine with removing User.login, I am just trying to say we need to replace it with something that allows for creation of api keys (or obtaining an API key) via the webservices API. Extra cool points for
having a webservice method for revoking an api key so that an App/client can "log out" the user and require them to get a new api key by entering username and password.

> ::: template/en/default/global/user-error.html.tmpl
> > @@ +103,5 @@
> > > +    correctly.
> > > +
> > > +  [% ELSIF error == "api_key_revoked" %]
> > > +    [% title = "Invalid API key" %]
> > > +    The API key you specified has been revoked by the user that created it.
> > 
> > Combine the two above to be a single api_key_invalid error. We don't want to
> > necessarily give away that a key has been revoked.
>  
> I disagree. It's about time we give user's helpful error messages. There is
> no security implications in letting a user know that a token has been
> revoked vs. not exists. It still won't work, and the API key by itself won't
> let you know who owns it.

That's fine. Was a suggestion.
 
dkl

Comment 46

[:glob ✱]

i agree with simon -- if we expose a method which accepts user/pass and returns a key, there's nothing stopping an application storing the user's credentials and creating a new key for each login (or request!).  this removes the control of the api keys out of the user's hands, and makes it impossible for a user to revoke access to bugzilla on a per-application basis.

if the workflow of copy and pasting a string from one browser tab to another is the primary concern, then we could look at a bugzilla hosted page which allows the user to authorise an application (aka generate a key) similar to how oauth works.  if this route is taken i suggest this be targeted at bugzilla 6 instead of blocking 5's release.

Comment 47

[Gervase Markham [:gerv]]

Use cases:

* Bugzilla itself - requests for data for our own web pages
* Individual installed clients - e.g. 
* Web pages anyone can access - e.g. live buglists on wiki.mozilla.org
* Bugzilla dashboards -

Comment 48

[Gervase Markham [:gerv]]

Urk. How did that happen? That comment wasn't even close to being finished :-( More soon.

Gerv

Comment 49

[Gervase Markham [:gerv]]

We should make API keys semi-expire after an admin-defined amount of time, with an error code that we document, and that key-using implementations should be written to understand. So if e.g. the admin sets the timeout at 6 months, then if someone doesn't use an API key for that long and then tries to use it, Bugzilla should send back a "not used for too long" error code, and the app can prompt him to log in to Bugzilla, visit the API key management UI, and click "reactivate" next to the API key. Clicking reactivate updates the last-used date to NOW(), and so removing the block.

This means we can reduce the risk of people creating API keys then forgetting they exist, thereby leaving a big back door into their account.

Also, we should not display the API keys themselves in the API key management UI after they've been used once. This prevents someone with short-term access to someone's Bugzilla account copying an existing key and thereby avoiding the "you have a new key!" email, but also avoids the Google "it's only on this one load of the web page this one time, and if you don't copy it right now, then you have to make a new one" problem. We can perhaps implement this by setting the last_used value to NULL until it's used once.

Gerv

Comment 50

[Gervase Markham [:gerv]]

I assume this update to the title is correct?

We should make sure that we don't have to rearchitect this entirely if we implement OAuth, and that OAuth-issued creds can be managed in the same way as our home-grown api_key system.

Gerv

Comment 51

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #50)
> We should make sure that we don't have to rearchitect this entirely if we
> implement OAuth, and that OAuth-issued creds can be managed in the same way
> as our home-grown api_key system.

we don't have any plans to use oauth with bugzilla; i don't think it's valuable to make that a consideration when designing this.

> I assume this update to the title is correct?

uhh, doesn't the plan call for _removing_ User.login ?

Comment 52

[Gervase Markham [:gerv]]

In the design doc, Simon raised the issue of token bloat. Are we using Bugzilla::Token::issue_hash_token() to create the tokens? If so, then there's no token bloat because those tokens are not added to the token table, because they can be validated using only the information they themselves contain.

Gerv

Comment 53

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #51)
> uhh, doesn't the plan call for _removing_ User.login ?

Yes, eventually, but by "user/pass" I meant as URL parameters on the request itself. AIUI, that capability is not going away.

Gerv

Comment 54

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #51)
> we don't have any plans to use oauth with bugzilla; i don't think it's
> valuable to make that a consideration when designing this.

Well, you just suggested it in comment #46 :-) And I'm sure we'll get pressure from creators of Bugzilla dashboards and the like to make their lives easier and their products less clunky.

Gerv

Comment 55

[Simon Green]

(In reply to Gervase Markham [:gerv] from comment #53)
> (In reply to Byron Jones ‹:glob› from comment #51)
> > uhh, doesn't the plan call for _removing_ User.login ?
> 
> Yes, eventually, but by "user/pass" I meant as URL parameters on the request
> itself. AIUI, that capability is not going away.

Correct. Specifying Bugzilla_login and Bugzilla_password is going to be supported for the long term. The User.login and User.logout function will be removed in Bugzilla 5.0+1. After all, that is the security problem we are trying to fix.

(In reply to Gervase Markham [:gerv] from comment #52)
> In the design doc, Simon raised the issue of token bloat. Are we using
> Bugzilla::Token::issue_hash_token() to create the tokens? If so, then
> there's no token bloat because those tokens are not added to the token
> table, because they can be validated using only the information they
> themselves contain.

No, we create new tokens per page load. They do auto expire after three days. For a site like Red Hat Bugzilla (which gets millions of web page loads per month) that isn't going to be an insignificant number of extra db activity.

I didn't know about hash tokens until now. The problem with this is that it would be possible to create a valid quasi API key (assuming the secret is known which would be true of most sites) for RPC calls, which can present a security risk. Even more so because the person that would exploit this would have a copy of the secret key on their PC.

(In reply to Gervase Markham [:gerv] from comment #50)
> I assume this update to the title is correct?

I don't think so. Calls from Bugzilla web UI don't use API keys or username/password for authentication.

(In reply to Gervase Markham [:gerv] from comment #49)
> We should make API keys semi-expire after an admin-defined amount of time,
> with an error code that we document, and that key-using implementations
...

I'm not opposed to this per-se, but think we can do this as a separate bug if desired. Ditto with dkl's generate API keys via the web UI. If that feature is wanted, it is not hard to do it once this code is committed.

Comment 56

[Gervase Markham [:gerv]]

(In reply to Simon Green from comment #55)
> No, we create new tokens per page load. They do auto expire after three
> days. For a site like Red Hat Bugzilla (which gets millions of web page
> loads per month) that isn't going to be an insignificant number of extra db
> activity.
> 
> I didn't know about hash tokens until now. The problem with this is that it
> would be possible to create a valid quasi API key (assuming the secret is
> known which would be true of most sites) for RPC calls, which can present a
> security risk. Even more so because the person that would exploit this would
> have a copy of the secret key on their PC.

I'm not quite following this. Can you lay out the security risk you see from using create_hash_token() to make the in-page tokens for Bugzilla API access? As you note above, the benefit would be great if we can find a way to use tokens which do not have to be stored in the DB.

Gerv

Comment 57

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #43)
> ::: Bugzilla/Auth/Login/Cookie.pm
> @@ +51,5 @@
> >              $user_id = $cookie->value if $cookie;
> >          }
> > +
> > +        # If the call is for a web service, we need to check an api token is
> > +        # provided. This is different from the token below
> 
> This currently breaks JSONRPC/XMLRPC using POST because of cookies being
> allowed for those call types.

We haven't allowed cookies for JSONRPC/XMLRPC since Bugzilla 4.4.3
 
> ::: Bugzilla/Token.pm
> @@ +34,5 @@
> > +# Create a token used for internal API authentication
> > +sub issue_apiauth_token {
> > +    # Generates a random token, adds it to the tokens table, and returns
> > +    # the token to the caller.
> > +    return _create_token(Bugzilla->user->id, 'apiauth', '');
> 
> s/apiauth/api_token/

Since the current maximum length of tokentype is 8, I've increased to 16.

Comment 58

[Simon Green]

Attachment: bug726696-v2.patch

Comment 59

[David Lawrence [:dkl]]

Comment on attachment 8455126 [details] [diff] [review]
bug726696-v2.patch

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

(In reply to Simon Green from comment #57)
> > This currently breaks JSONRPC/XMLRPC using POST because of cookies being
> > allowed for those call types.
> 
> We haven't allowed cookies for JSONRPC/XMLRPC since Bugzilla 4.4.3

Well technically a cookie is allowed for auth if one is provided. We just no longer return cookies
when a user successfully logs in using webservices either via User.login or passing username/password to a method. 

http://git.mozilla.org/?p=bugzilla/bugzilla.git;a=blob;f=Bugzilla/WebService/Server/JSONRPC.pm;h=5290caa5d08e858d0116bfa2864878cfa02a83cc;hb=ca7b39aa66#l244 [github]

But your changes in Bugzilla/Auth/Login/Cookie.pm breaks XMLRPC/JSONRPC when tokens are used as it will generate an error if i_am_webservice() is true. The 4.4 QA webservice tests use tokens and they are currently all broken with this patch. We could change Bugzilla/Auth/Login/Cookie.pm to the following and it would work for webservice clients passing in token= or Bugzilla_token= 

-        if (i_am_webservice()) {
+        if (i_am_webservice() && !$self->login_token) {

or some other similar solution. I realize token support is deprecated but it still needs to work for the time being.

Also when a client does not pass a token, api_key, login and/or password, for a normal webservice call they get the following error which is misleading:

'When making internal API calls, you must specify a token with the Bugzilla_api_token key.'

since you are calling the 'auth_no_token' error when the Bugzilla_api_token param is not present. This error needs to be tailored based on whether the client is using the WebUI for ajax calls or doing normal webservice calls via REST/XMLRPC/JSONRPC. 'auth_no_token' for WebUI calls and 'api_key_not_valid' or similar for normal webservices.

dkl

Comment 60

[Simon Green]

Attachment: bug726696-v3.patch

Fixed the issues with token use and anonymous RPC calls.

Comment 61

[David Lawrence [:dkl]]

Comment on attachment 8459285 [details] [diff] [review]
bug726696-v3.patch

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

r=dkl

Comment 62

[David Lawrence [:dkl]]

Sorry for not a? this right after the r+. If glob can approve this tonight, and you check it in, we can get it into the 4.5.5 devel snapshot going out soon. Otherwise it will go out with the 5.0 RC1 coming out soon.

Comment 63

[David Lawrence [:dkl]]

We will branch for 5.0 after you commit this.

dkl

Comment 64

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   9f0f44b..fd29ee5  master -> master

Comment 65

[Frédéric Buclin]

Added to relnotes for 5.0rc1.