Closed Bug 1184332 Opened 9 years ago Closed 9 years ago

Add Restricted API calls for MozReview

Categories

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, defect)

Production
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dylan, Assigned: dylan)

References

Details

(Keywords: bmo-goal)

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 file, 3 obsolete files)

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.
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)
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)
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.
User Story: (updated)
Keywords: bmo-goal
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)
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.
User Story: (updated)
Attached patch 1184332_1.patch (obsolete) — Splinter Review
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)
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-
(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!
Attached patch 1184332_3.patch (obsolete) — Splinter Review
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.
Attached patch 1184332_4.patch (obsolete) — Splinter Review
> 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-
Attached patch 1184332_6.patchSplinter Review
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+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   589ec37..fa7ae98  master -> master
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
See Also: → 1209625
Product: bugzilla.mozilla.org → bugzilla.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: