Add Restricted API calls for MozReview

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: MozReview Integration
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dylan, Assigned: dylan)

Tracking

({bmo-goal})

Production
bmo-goal

Details

User Story

User.get()
User.login()
User.valid_login()
Bug.add_comment()
Bug.add_attachment()
Bug.attachments()
Bug.get()
Bug.update_attachment()

Will add the multi-attachment method in a different bug, it doesn't belong here.

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
This feature will simply be a set of additional RPC methods / REST resources (we will do both as mozreview is currently using XMLRPC) that check that the API key used for authentication is one issued to MozReview.

This will require some mechanism for labeling API keys, and I am strongly in favor of NOT using the description field. Another bug will be filed for that piece.
(Assignee)

Comment 1

2 years ago
Please provide a list of API methods MozReview needs to be useful. We can add additional methods as needed, but I must know what is required for a MVP.
Flags: needinfo?(mcote)

Comment 2

2 years ago
After we switch to API keys, we'll be using the following (XML-RPC, for now) calls:

User.get()
User.login()
User.valid_login()
Bug.add_comment()
Bug.add_attachment()
Bug.attachments()
Bug.get()
Bug.update_attachment()

In some of those calls I would love to be able to further restrict things, e.g., restricting attachment calls to dealing with x-reviewboard-request mimetypes.  But we can add that later.
Flags: needinfo?(mcote)

Comment 3

2 years ago
We could also use some API changes, e.g. to allow multiple attachment operations in a single call, that we could add on to this work if we didn't want to make general API changes at this time.
(Assignee)

Updated

2 years ago
User Story: (updated)
(Assignee)

Updated

2 years ago
Keywords: bmo-goal
(Assignee)

Comment 4

2 years ago
First proposal: Can we use the app_id field of the API keys to identify moz review API keys?
It is a sha2 hash of (callback_url + description). I would add a param (mozreview_app_id) and add a check to Bugzilla->login() (when called from a web-service) to see if the current API key has that app_id, and reject the request unless it is for a MozReview* resource.
Flags: needinfo?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #4)
> First proposal: Can we use the app_id field of the API keys to identify moz
> review API keys?
> It is a sha2 hash of (callback_url + description).

assuming you mean callback_url + app_id, this sounds like a reasonable approach :)
Flags: needinfo?(glob)
(Assignee)

Comment 6

2 years ago
To clarify, the app_id is a hash of the callback_url and the callback site's description field. The important thing here is all api keys from the delegation API will have this set, and we could simply have the one from MozReview (or a list if the description of MozReview changes) to match the incoming API keys from it.
(Assignee)

Updated

2 years ago
User Story: (updated)
(Assignee)

Comment 7

2 years ago
Created attachment 8639649 [details] [diff] [review]
1184332_1.patch

It seemed silly to duplicate all the methods (and potentially error prone), since I already have to check every API call anyway.

There happens to be a hook for this, so I used that rather than modifying handle_login() in Bugzilla::WebService::Server. This makes dkl's deliverable easier, but if needed I can move it. 

:mcote wanted a method to update multiple attachments, and I think that's a reasonable API that we should have, so I'll file another bug for it.
Attachment #8639649 - Flags: review?(glob)
(Assignee)

Comment 8

2 years ago
d'oh! Disregard the WebService.pm code, that was left over from the first impl, with a "test" stub method I used for testing that the API blocking worked. :-)
Comment on attachment 8639649 [details] [diff] [review]
1184332_1.patch

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

this looks good, but please attach an updated patch without the accidental changes for review.

::: extensions/MozReview/Extension.pm
@@ +80,3 @@
>      my ($self, $args) = @_;
> +    my $modules = $args->{panel_modules};
> +    $modules->{MozReview} = "Bugzilla::Extension::MozReview::Config";

template/en/default/hook/admin/params/editparams-current_panel.html.tmpl needs updating given the panel name change and to add a description for the new param.
Attachment #8639649 - Flags: review?(glob) → review-
(Assignee)

Comment 10

2 years ago
(In reply to Byron Jones ‹:glob› from comment #9)
> Comment on attachment 8639649 [details] [diff] [review]
> 1184332_1.patch
> 
> Review of attachment 8639649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks good, but please attach an updated patch without the accidental
> changes for review.
> 
> ::: extensions/MozReview/Extension.pm
> @@ +80,3 @@
> >      my ($self, $args) = @_;
> > +    my $modules = $args->{panel_modules};
> > +    $modules->{MozReview} = "Bugzilla::Extension::MozReview::Config";
> 
> template/en/default/hook/admin/params/editparams-current_panel.html.tmpl
> needs updating given the panel name change and to add a description for the
> new param.

editparams-current_panel.html.tmpl is removed by this patch. The descriptions are in extensions/MozReview/template/en/default/admin/params/mozreview.html.tmpl which is added. I'll fix up the rest. Thanks!
(Assignee)

Comment 11

2 years ago
Created attachment 8640063 [details] [diff] [review]
1184332_3.patch

Revised patch
Attachment #8639649 - Attachment is obsolete: true
Attachment #8640063 - Flags: review?(glob)
Comment on attachment 8640063 [details] [diff] [review]
1184332_3.patch

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

after hitting 'accept' on the auth delegation form:
Undefined subroutine &Bugzilla::User::APIKey::_check_app_id called at Bugzilla/Object.pm line 721.

the 'auth delegation request' page has the following text:
> Do you want this website to have *complete* access to your Bugzilla account
this should be changed for mozreview delegation requests so indicate the limited set of perms mozreview has.

::: Bugzilla/Auth/Login/APIKey.pm
@@ +32,5 @@
> +sub app_id {
> +    my ($self) = @_;
> +
> +    return $self->{app_id};
> +}

nittest of nits: remove the blank line here, to match set_app_id (or add a blank line to set_app_id)

::: extensions/MozReview/Extension.pm
@@ +25,5 @@
> +    'Bug.add_attachment'    => 1,
> +    'Bug.attachments'       => 1,
> +    'Bug.get'               => 1,
> +    'Bug.update_attachment' => 1,
> +);

change this to an array and use 'any'.

::: extensions/MozReview/lib/Config.pm
@@ +32,5 @@
> +                my ($url) = (@_);
> +
> +                return 'must be an HTTP/HTTPS absolute URL' unless $url =~ m{^https?://};
> +                return '';
> +            }

this checker appears to function the same as check_urlbase, so you should be able to use check_urlbase here instead.
Attachment #8640063 - Flags: review?(glob) → review-
(In reply to Byron Jones ‹:glob› from comment #12)
> the 'auth delegation request' page has the following text:
> > Do you want this website to have *complete* access to your Bugzilla account
> this should be changed for mozreview delegation requests so indicate the
> limited set of perms mozreview has.

This isn't necessary, since mozreview is a "trusted" app and hence this page is never shown.
(Assignee)

Comment 14

2 years ago
Created attachment 8641971 [details] [diff] [review]
1184332_4.patch

> after hitting 'accept' on the auth delegation form:
> Undefined subroutine &Bugzilla::User::APIKey::_check_app_id called at
> Bugzilla/Object.pm line 721.
Oy. A victim of diffing this. There's a second version of this patch that includes the change to use POST requests (so I could test against my flask app). The new patch checks this (and adds a code-error check).


> the 'auth delegation request' page has the following text:
> > Do you want this website to have *complete* access to your Bugzilla account
> this should be changed for mozreview delegation requests so indicate the
> limited set of perms mozreview has.
as mcote points out, this should not be seen if mozreview is configured in the params.

> ::: extensions/MozReview/lib/Config.pm
> @@ +32,5 @@
> > +                my ($url) = (@_);
> > +
> > +                return 'must be an HTTP/HTTPS absolute URL' unless $url =~ m{^https?://};
> > +                return '';
> > +            }
> 
> this checker appears to function the same as check_urlbase, so you should be
> able to use check_urlbase here instead.

check_urlbase requires a trailing slash, and this doesn't care. I haven't made this change because of the different behavior.
Attachment #8640063 - Attachment is obsolete: true
Attachment #8641971 - Flags: review?(glob)
Comment on attachment 8641971 [details] [diff] [review]
1184332_4.patch

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

most of this looks good.

unauthenticated requests are failing:
eg. /rest/time
> Can't call method "can" on an undefined value at ./extensions/MozReview/Extension.pm line 94
eg. rest/bug/888
> Can't call method "app_id" on an undefined value at ./extensions/MozReview/Extension.pm line 96

all rest requests are rejected, even those in the whitelist:
eg. /rest/bug/888?api_key=XXX
> The requested method 'Bugzilla::WebService::Bug.get' was not found.
(interesting that it's 'Bugzilla::WebService::Bug.get' and not just 'Bug.get')
Attachment #8641971 - Flags: review?(glob) → review-
(Assignee)

Comment 16

2 years ago
Created attachment 8642796 [details] [diff] [review]
1184332_6.patch

Fixing the bz_class_name() was... more involved than I could justify. This change is much easier. This works for rest methods and does not break unauthenticated API calls.
Attachment #8641971 - Attachment is obsolete: true
Attachment #8642796 - Flags: review?(glob)
Comment on attachment 8642796 [details] [diff] [review]
1184332_6.patch

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

r=glob
Attachment #8642796 - Flags: review?(glob) → review+
(Assignee)

Comment 18

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   589ec37..fa7ae98  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
See Also: → bug 1209625
You need to log in before you can comment on or make changes to this bug.