1144468 - Bugzilla Auth Delegation via API Keys

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P1)

Description

[Mark Côté [:mcote]]

This feature builds on the new API keys.

API keys are great in that they allow specific third-party applications to access Bugzilla with your account without needing your password, and they provide a route towards site-specific permissions.

API keys are not great as they could be because you need to generate one per application from a special preferences screen, and they are long, requiring cutting and pasting.  There's a proposal for better mobile-application support (bug 1138772) but none for desktop.

We can fix the desktop situation with a simple authorization interface.  The user story is as follows:

1. User wishes to log into a Bugzilla-based third-party web application.
2. Application redirects to a special Bugzilla login screen.
3. User logs in with their usual Bugzilla username and password.
4. If the user has never authorized this particular application before, they are prompted to allow the application access to their account.
4a. Bugzilla automatically creates an API key.
5. Bugzilla redirects the user back to the application, in parallel sending along the API key.
6. Application uses the API key in any future transactions in which access as the user is required.
7. If the application ever finds that the API key is no longer valid, user is logged out.  They would be required to generate a new API key to log back in.

There are a few open questions (in my mind, anyway), including

a. How is the key later identified as being associated with the application (4a above), so that the user knows which one to disable if they later desire to (and possibly in order to be sent back to the application; see b. below)?  Perhaps something sent along with the initial redirect to Bugzilla (2)?
b. Should we always send the key back to the application when the user logs into that app (e.g. if their session on the app expired), or only the first time (5)?
c. How is the key sent back to the application (5)?

I'm sure there are various other details to work out as well.

The initial candidate is MozReview, but perhaps BzDeck will want to try it out too.

Comment 1

[Kohei Yoshino [:kohei]]

(In reply to Mark Côté [:mcote] from comment #0)
> a. How is the key later identified as being associated with the application
> (4a above), so that the user knows which one to disable if they later desire
> to (and possibly in order to be sent back to the application; see b. below)?
> Perhaps something sent along with the initial redirect to Bugzilla (2)?

Maybe Bugzilla would have a certified a certified app list, and an app just pass a query string like this?

BzDeck:
> let bzwin = window.open('https://bugzilla.mozilla.org/userprefs.cgi? \
>                          tab=apikey&action=generate&app=bzdeck', 'bzwin');

> c. How is the key sent back to the application (5)?

How about MessageEvent? The postMessage method can specify a target origin, so even if other app uses the link above, that attempt will fail.

https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage

I haven't tested it yet but the code may look like this:

Bugzilla:
> window.opener.postMessage('APIKEY:9PSm7BbOJ9dDgHOorqKos11wMPNP2PG7eBhpnJVb',
>                           'https://www.bzdeck.com');

BzDeck:
> window.addEventListener(event => {
>   if (event.source === bzwin &&
>       event.origin === 'https://bugzilla.mozilla.org') {
>     // Got the key as event.data
>     bzwin.close();
>   }
> });

Comment 2

[Kohei Yoshino [:kohei]]

Hmm, it may work with web apps but won't work with native mobile apps. I'm not familiar with this area but standard methods like OAuth might be better?

Comment 3

[Kohei Yoshino [:kohei]]

Any restrictions on Firefox OS regarding those ideas?

Comment 4

[Mark Côté [:mcote]]

OAuth might be better, but we have API keys now.  We can think about that later if we can't come up with a good story for mobile native apps.

Comment 5

[Mark Côté [:mcote]]

Also, something else occurred to me.  Given that MozReview is tightly coupled to Bugzilla, and we want to encourage people to use it, I think we should add an option when creating an account to automatically create and send an API key for MozReview.  It would default to true.  This would remove one step when a new user has to create a Bugzilla account in order to import a GitHub PR.

Comment 6

[:glob ✱]

(In reply to Mark Côté [:mcote] from comment #5)
> Also, something else occurred to me.  Given that MozReview is tightly
> coupled to Bugzilla, and we want to encourage people to use it, I think we
> should add an option when creating an account to automatically create and
> send an API key for MozReview.  It would default to true.  This would remove
> one step when a new user has to create a Bugzilla account in order to import
> a GitHub PR.

mozreview is tightly coupled to _bmo_, not bugzilla.  this system should allow for this ability, either by design with approved systems, or to enable it with hooks.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Attachment: BADPUN.html

Here's my plan for implementing this bug. Comments greatly appreciated -- pretty sure some things are a bit too hand wavey. 

As already discussed with glob, perhaps an application registry not called for. The only change in requirement is that the third party application will need to callback into Bugzilla to verify that the Bugzilla login name (email) is correct.

Relatedly, this documented API will need to be fixed for this to work:

https://bugzilla.readthedocs.org/en/latest/api/core/v1/user.html#valid-login

Comment 8

[:glob ✱]

> This extension will not initially support upstream Bugzilla. 

this is an upstream core bug, and this work should be part of bugzilla itself, not in an extension.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8592046 [details]
BADPUN.html

Clearing feedback as dylan mentioned he had a new version of the plan coming.

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Obviously I should never not be using modal...

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Progress in this is going along quite nicely. I will have a working auth.cgi end point ready to show off next week, possibly a first round review ready by Thursday.

I would sort of like to make API key descriptions "click to edit", to avoid accidentally changing them. Would such a patch be acceptable? If so, I can file a bug.

Comment 12

[David Lawrence [:dkl]]

I am fine with this change as we do the same in may places such as show_bug.cgi already. File an upstream bug and I can review it.

dkl

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Alright, current difficulties: 

For some reason, possibly relating to the initial design having a white list, I overlooked
the need to prevent a malicious third party passing a confirmed=1 flag and skipping the confirmation step.
I've waffled between doing some form URL signing (using the site wide secret) but eventually realized the short comings of this approach... The right solution is to generate a new type of token.

How this works is simple:

1) when auth.cgi is loaded from a GET request, on the first go-round, we create a token (tokentype = auth_delegation) and include it in the confirmation screen. The token is associated with the user id and will expire normally. If the user accepts the delegation request, we POST to auth.cgi with the token.
2) when auth.cgi is loaded from a POST request, we check that the token matches and redirect to the third party with their API key.

I've tested this approach with a fairly simple Flask app (acting as my third party) and the results are good. What's left? The error handling is incomplete and sanity tests don't pass as the errors we *do* throw are not yet described.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1144468_1.patch

First version of the BAD patch. There is a skeletal amount of documentation. A good test of it is if you can understand how to make it spit out an API key.

Some notes:

1) It sends an email when an API key is created (I feel this is important for security)
2) There is no longer a "rejection" state that the calling application gets.

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 8601218 [details] [diff] [review]
1144468_1.patch

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

1. Docs Error:
/home/bugzilla/devel/htdocs/1144468/docs/en/rst/using/preferences.rst:140: WARNING: duplicate label api-keys, other instance in /home/bugzilla/devel/htdocs/1144468/docs/en/rst/integrating/apikeys.rst
2. Spoke in IRC about possibly using "Authentication Delegation" for outfacing text. Fine to leave internal naming to use shorter 'auth' such as auth.cgi.

Otherwise works well and should be a short r+ 

dkl

::: auth.cgi
@@ +69,5 @@
> +
> +    MessageToMTA($message);
> +
> +    $callback_uri->query_param(bz_api_key => $new_key->api_key);
> +    $callback_uri->query_param(bz_login => $user->login);

nit: use client_api_key and client_api_login. Not a fan of 'bz' :)

::: template/en/default/account/auth/delegation.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% PROCESS global/header.html.tmpl
> +title = "Auth Delegation Request" %]

indent

@@ +10,5 @@
> +title = "Auth Delegation Request" %]
> +
> +<h1>[% title FILTER html %] </h1>
> +<p>
> +  A third-party webiste (<a href="[% callback_base FILTER html %]">[% callback_base FILTER html %]</a>)

s/webiste/website/

@@ +12,5 @@
> +<h1>[% title FILTER html %] </h1>
> +<p>
> +  A third-party webiste (<a href="[% callback_base FILTER html %]">[% callback_base FILTER html %]</a>)
> +    would like to have <strong>complete</strong> access to your [% terms.Bugzilla %] account.
> +</p>

nit: empty line between paragraphs

::: template/en/default/global/user-error.html.tmpl
@@ +122,5 @@
> +    This site is does not have auth delegation enabled.
> +    Please contact an administrator if you require this functionality.
> +
> +  [% ELSIF error == "auth_delegation_missing_callback" %]
> +    [% title = "Auth delegation impossible without callback uri" %]

s/uri/URI/

@@ +124,5 @@
> +
> +  [% ELSIF error == "auth_delegation_missing_callback" %]
> +    [% title = "Auth delegation impossible without callback uri" %]
> +    It looks like auth delegation was attempted, but no callback URI was passed.
> +    You were sent here by some other site, please contact them for details.

s/details/support/

@@ +129,5 @@
> +
> +  [% ELSIF error == "auth_delegation_missing_description" %]
> +    [% title = "Auth delegation impossible without description" %]
> +    It looks like auth delegation was attempted, but no description was passed.
> +    You were sent here by some other site, please contact them for details.

ditto

Comment 16

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1144468_2.patch

With fixes + improved-ish documentation. I suspect we'll want to iterate on that at least once more, how does it look now? 

Note: when $skip_confirmation is true, we skip checking the $token param. The previous version didn't do that.

Also, "make clean" deletes everything in the documentation directory. That's not very good...

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 8604270 [details] [diff] [review]
1144468_2.patch

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

New doc build error:

/home/bugzilla/devel/htdocs/1144468/docs/en/rst/integrating/auth-delegation.rst:: WARNING: document isn't included in any toctree

Need to add the new file to docs/en/rst/integrating/index.rst

::: auth.cgi
@@ +47,5 @@
> +             callback_base     => $callback_base );
> +
> +Bugzilla::Hook::process('auth_delegation_confirm', \%args);
> +
> +if (lc($cgi->request_method) eq 'post' || $skip_confirmation) {

Didn't catch this in the first review, but if systemA redirects to auth.cgi on systemB (Bugzilla) and the user is not yet logged into systemB, you get the login screen for systemB. Once you type in your username/password and submit, you get the missing token error instead of getting the 'Accept' page. The login form does a POST so it is getting trapped above. Maybe use a different hidden variable to specify to validate the token, etc.

dkl

::: docs/en/rst/api/core/v1/general.rst
@@ +110,4 @@
>  
>  You can specify ``Bugzilla_api_key`` or simply ``api_key`` as an argument to
>  any call, and you will be logged in as that user if the key is correct and has
> +not been revoked. You can set up an API key by using the :ref:`API key <api-keys>` tab in the

API Keys tab

::: docs/en/rst/integrating/auth-delegation.rst
@@ +16,5 @@
> +and the Bugzilla site is `http://bugs.example.org`.
> +
> +1. Provide a link or redirect the user to `http://bugs.example.org/auth.cgi?callback=http://app.example.org/callback&description=app%description`
> +2. Assuming the user is agreeable, they will be redirected to `http://app.example.org/callback` via a GET request
> +   with two additional parameters: `bz_api_key` and `bz_login`. If your callback URL contained

s/bz_/client_/

@@ +20,5 @@
> +   with two additional parameters: `bz_api_key` and `bz_login`. If your callback URL contained
> +   any additional query string parameters, these will also be returned to you. It is a good
> +   idea to include some sort of token to ensure that your app originated this request.
> +
> +   The description is used to populate the description field on the 'API Key' tab in the Preferences page.

s/Key/Keys/

@@ +22,5 @@
> +   idea to include some sort of token to ensure that your app originated this request.
> +
> +   The description is used to populate the description field on the 'API Key' tab in the Preferences page.
> +3. Finally, you should check the the API key and login are valid, using the :ref:`rest_user_valid_login` REST
> +   resource.

rest_user_valid_login validates the 'old-style' tokens and not the newer API keys so leave this out. AFAIK we do not have an API
method for validating API keys yet.

Comment 18

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Lawrence [:dkl] from comment #17)
> Comment on attachment 8604270 [details] [diff] [review]
> 1144468_2.patch
> 
> Review of attachment 8604270 [details] [diff] [review]:
> -----------------------------------------------------------------
> rest_user_valid_login validates the 'old-style' tokens and not the newer API
> keys so leave this out. AFAIK we do not have an API
> method for validating API keys yet.

Consider it an accidental feature, but valid_login does work.

When api_key is a query parameter & valid_login is called, it calls Bugzilla->login.
That results in (eventually) get_login_info() being called.

https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/Bugzilla/Auth/Login/APIKey.pm#L49 get_login_info() returns a user id, and the Auth stack populate Bugzilla->user.

valid_login then checks if 'login' param matches Bugzilla->user->login, and returns a { result: true }
or { result: false }.

https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/Bugzilla/WebService/User.pm#L99-L108

I'll have revised docs and fixed up in the morn'.

Comment 19

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1144468_5.patch

Fixed typos, docs. Added a note about validating callback uris. Going to try publishing my python demo-app today, which might be good for you to look over to see how it all fits together?

Comment 20

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1144468_5.patch

It's too much to ask for a file copy operation to not somehow involve gzip. #emacsproblems.

Comment 21

[Mark Côté [:mcote]]

Comment on attachment 8607024 [details] [diff] [review]
1144468_5.patch

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

::: docs/en/rst/integrating/auth-delegation.rst
@@ +9,5 @@
> +
> +Authentication Flow
> +-------------------
> +
> +The authentication process begins by directing the user to Bugzilla site's auth.cgi.

I think you want need a "the": "to the Bugzilla site's auth.cgi"

@@ +16,5 @@
> +
> +1. Provide a link or redirect the user to `http://bugs.example.org/auth.cgi?callback=http://app.example.org/callback&description=app%description`
> +2. Assuming the user is agreeable, they will be redirected to `http://app.example.org/callback` via a GET request
> +   with two additional parameters: `client_api_key` and `client_api_login`.
> +3. Finally, you should check the the API key and login are valid, using the :ref:`rest_user_valid_login` REST

"that the API key"

@@ +20,5 @@
> +3. Finally, you should check the the API key and login are valid, using the :ref:`rest_user_valid_login` REST
> +   resource.
> +
> +If your callback URL contained any additional query string parameters, these
> +will also be returned to you. Your callback URL must contain a checksum of the callback url (excluding the checksum itself)

This isn't clear, to me at least... how does "my callback URL" contain a checksum of "the callback url" (also you use both "url" and "URL" in this file).

@@ +26,5 @@
> +
> +The description should include the name of your application, in a form that will be recognizable to users.
> +This description is used in the :ref:`API Keys tab <api-keys>` in the Preferences page.
> +
> +The API Key passed to the callback will be valid until the user revokes it.

You don't capitalize "key" elsewhere in this file.

::: template/en/default/global/user-error.html.tmpl
@@ +118,5 @@
>      created.
>  
> +  [% ELSIF error == "auth_delegation_disabled" %]
> +    [% title = "Can't use auth delegation" %]
> +    This site is does not have auth delegation enabled.

Extra "is" here.

@@ +124,5 @@
> +
> +  [% ELSIF error == "auth_delegation_missing_callback" %]
> +    [% title = "Auth delegation impossible without callback URI" %]
> +    It looks like auth delegation was attempted, but no callback URI was passed.
> +    You were sent here by some other site, please contact them for support.

Semicolon instead of comma (also occurs below).

Comment 22

[Mark Côté [:mcote]]

Comment on attachment 8607024 [details] [diff] [review]
1144468_5.patch

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

::: docs/en/rst/api/core/v1/general.rst
@@ +114,3 @@
>  Preferences pages.
>  
> +API Keys may also be requested via :ref:`Authentication Delegation <auth-delegation>`.

"Keys" should not be capitalized unless referring to the API Keys preferences tab.

Comment 23

[David Lawrence [:dkl]]

Comment on attachment 8607024 [details] [diff] [review]
1144468_5.patch

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

mcotes changes on commit?. r=dkl

::: auth.cgi
@@ +48,5 @@
> +
> +Bugzilla::Hook::process('auth_delegation_confirm', \%args);
> +
> +my $confirm = $cgi->param('confirm');
> +my $did_confirmation = lc($cgi->request_method) eq 'post' && $confirm;

nit: my $confirmed = lc($cgi->request_method) eq 'post' && $cgi->param('confirm');

Comment 24

[Frédéric Buclin]

Comment on attachment 8607024 [details] [diff] [review]
1144468_5.patch

>+++ b/template/en/default/account/auth/delegation.html.tmpl

>+    <input type="hidden" name="callback" value="[% callback FILTER none %]">
>+    <input type="hidden" name="description" value="[% description FILTER none %]">
>+    <input type="hidden" name="token" value="[% token FILTER none %]">

You must use |FILTER html| for all of them. At least the description is not sanitized and allows XSS. And in case someone is able to abuse auth.cgi, he could inject data into callback and token too.

Comment 25

[David Lawrence [:dkl]]

Comment on attachment 8607024 [details] [diff] [review]
1144468_5.patch

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

(In reply to Frédéric Buclin from comment #24)
> Comment on attachment 8607024 [details] [diff] [review]
> 1144468_5.patch
> 
> >+++ b/template/en/default/account/auth/delegation.html.tmpl
> 
> >+    <input type="hidden" name="callback" value="[% callback FILTER none %]">
> >+    <input type="hidden" name="description" value="[% description FILTER none %]">
> >+    <input type="hidden" name="token" value="[% token FILTER none %]">
> 
> You must use |FILTER html| for all of them. At least the description is not
> sanitized and allows XSS. And in case someone is able to abuse auth.cgi, he
> could inject data into callback and token too.

Good catch on that LpSolit. Please attach a revised patch with that and the other suggested changes.

dkl

::: auth.cgi
@@ +48,5 @@
> +
> +Bugzilla::Hook::process('auth_delegation_confirm', \%args);
> +
> +my $confirm = $cgi->param('confirm');
> +my $did_confirmation = lc($cgi->request_method) eq 'post' && $confirm;

nit: my $confirmed = lc($cgi->request_method) eq 'post' && $cgi->param('confirm');

Comment 26

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1144468_6.patch

Youch! Good job spotting the filtering error, LpSolit!

That fix + typos are corrected.

Comment 27

[David Lawrence [:dkl]]

Comment on attachment 8607838 [details] [diff] [review]
1144468_6.patch

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

fix on commit. r=dkl

::: auth.cgi
@@ +48,5 @@
> +
> +Bugzilla::Hook::process('auth_delegation_confirm', \%args);
> +
> +my $confirmed = $cgi->param('confirm');
> +my $did_confirmation = lc($cgi->request_method) eq 'post' && $confirmed;

nit: my $confirmed = lc($cgi->request_method) eq 'post' && $cgi->param('confirm');

@@ +50,5 @@
> +
> +my $confirmed = $cgi->param('confirm');
> +my $did_confirmation = lc($cgi->request_method) eq 'post' && $confirmed;
> +
> +if ($did_confirmation || $skip_confirmation) {

if ($confirmed || $skip_confirmation) {

Comment 28

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   42d961c..d8cbd5b  master -> master

Comment 29

[Kohei Yoshino [:kohei]]

Some may already know but I have just implemented Auth Delegation support in my BzDeck app. That's awesome. Thanks so much Bugzilla folks!

https://github.com/bzdeck/bzdeck/commit/f4b25aa