1113208 - [rbbz] Endpoint for restricting requests associated with a given bug

Status: RESOLVED WONTFIX

(MozReview Graveyard :: General, defect)

Description

[Mark Côté [:mcote]]

It would be handy to have a custom endpoint that deleted (not closed, but fully deleted) a parent *and* all its children.

Comment 1

[Steven MacLeod [:smacleod]]

This shouldn't be very difficult. RB extensions can easily hook in their own custom API endpoints. I don't know if I'll have time to take care of this myself but I can definitely mentor this for anyone who wants to fix it.

Mark, what credentials is the current bugzilla extension which calls delete on the parent request using? Could you provide a link to the bugzilla code side or the bug that implemented it?

Comment 2

[Mark Côté [:mcote]]

It's currently using the admin account, which I would gladly change to an account with fewer permissions (although deleting any review request requires a lot of power no matter what).

Link to GitHub mirror for easy browsing (canonical source is on git.mozilla.org): https://github.com/mozilla/webtools-bmo-bugzilla/tree/master/extensions/Push/lib/Connector/ReviewBoard

Comment 3

[Mark Côté [:mcote]]

Attachment: MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Delete all children review requests when parent is deleted.
/r/2861 - Bug 1113208 - Test for deleting parent review request.

Pull down these commits:

hg pull review -r 752b12a2abdb29a6a18f549fc46952c7453cb599

Comment 4

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Delete all children review requests when parent is deleted.
/r/2861 - Bug 1113208 - Test for deleting parent review request.

Pull down these commits:

hg pull review -r 9244a137c1d0b8e4f05d924d92dba5a30d344b02

Comment 5

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2859/#review2183

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/rbbz/rbbz/extension.py

Comment 6

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/2861/#review2185

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2859/#review2313

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> -def on_draft_pre_delete(sender, instance, using, **kwargs):
> -    """ Handle draft discards.
> +def on_pre_delete(sender, instance, using, **kwargs):
> +    """Handle deletion of various models.
>  
>      There are no handy signals built into Review Board (yet) for us to detect
> -    when a squashed Review Request Draft is discarded. Instead, we monitor for
> -    deletions of models, and handle cases where the models being deleted are
> +    when Review Board resources are deleted. Instead, we monitor for deletions
> +    of models, and handle different cases.
> -    ReviewRequestDrafts. We then do some processing to ensure that the draft
> -    is indeed a draft of a squashed review request that we want to handle,
> -    and then propagate the discard down to the child review requests.
>      """
> -    if not sender == ReviewRequestDraft:
> +    if sender == ReviewRequestDraft:
> +        on_review_request_draft_pre_delete(instance)
> +    elif sender == ReviewRequest:
> +        on_review_request_pre_delete(instance)

I would prefer we use two signal hooks which at the top decide if they should run, rather than the single hook that forks.

I believe it makes the code cleaner going forward especially as the number of signals we capture grows.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +    for child in gen_child_rrs(review_request):
> +        child.delete()
> +
> +    for child in gen_rrs_by_extra_data_key(review_request, 'unpublished_rids'):
> +        child.delete()

Have you tested this when the children have drafts? What happens in that case?

Comment 8

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2861/#review2315

::: hgext/reviewboard/tests/test-review-request-delete.t
(Diff revision 2)
> +

I'd like us to also test this with drafts present on both the parent and child review requests.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review2401

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +    review_request.save()

Is it necessary to do this last bit? We're fully deleting this review request - I don't see much point in updating the extra data.

::: hgext/reviewboard/tests/test-review-request-delete.t
(Diff revision 2)
> +  $ commonenv rb-test-review-request-delete

According to gps, we don't need the argument to commonenv anymore.

::: hgext/reviewboard/tests/test-review-request-delete.t
(Diff revision 2)
> +  $ rbmanage stop rbserver

This can now be

```mozreview stop```

Comment 10

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Delete all children review requests when parent is deleted.
/r/2861 - Bug 1113208 - Test for deleting parent review request.

Pull down these commits:

hg pull review -r 525550d19bc3d1fec8335ff7fa4ac42491f0d295

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review3023

This looks good to me, with just one nit.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> +        # As above.

I don't think this comment adds much.

Comment 12

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/2857/#review3047

> I don't think this comment adds much.

No?  If I were to look at this function on its own and wonder why we're using such a wide except clause, it might take me a few minutes to realize that the same hack is present, and commented, in another function.  Or are you saying it should be expanded?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/2857/#review3053

> No?  If I were to look at this function on its own and wonder why we're using such a wide except clause, it might take me a few minutes to realize that the same hack is present, and commented, in another function.  Or are you saying it should be expanded?

Yeah, expanding the comment is fine too - just on its own, it didn't clear anything up for me. Maybe make direct reference to discard_draft's comment.

Comment 14

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Delete all children review requests when parent is deleted.
/r/2861 - Bug 1113208 - Test for deleting parent review request.

Pull down these commits:

hg pull review -r 37c1a6ee2e942a9e3752cc4d49362fe529c79fc0

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review3055

Ship It!

Comment 16

[Steven MacLeod [:smacleod]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review3067

As discussed in IRC, this won't actually clean up all of the data we have stored. There are dependent objects like diffsets, reviews, etc. that may not be deleted.

The concrete case I can think of is any review request that was part of the set, but was discarded because the commit wasn't in the latest push, will be left hanging around. We won't actually have the data needed to remove that request until Bug 1125473 lands.

Don't Ship-It.

Comment 17

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Web API endpoint to restrict review-request visibility.

Pull down this commit:

hg pull review -r d761cbdb1bfaea82677c2d4c81e179874f4cb4ef

Comment 18

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/2857/#review3089

Hm there's an empty pylib/mozreview/mozreview/resources/__init__.py file in the diff that is apparently not visible here for some reason.

Comment 19

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/2857/#review3091

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 5)
> +    def get(self, request, *args, **kwargs):
> +        return 200, {}

I'm sure this isn't needed, but I couldn't figure out how to access the POST method from RBClient without it, since root.get_path() fails unless there's a GET handler.  I wanted to post this for review nonetheless, so just let me know what I'm doing wrong. :)

Comment 20

[Mark Côté [:mcote]]

The new approach is to restrict visibility on all review requests and drafts with extra_data identifiers beginning with bz://<bug>/, as well as removing any "Bug X" commits from the review repo.

Comment 21

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2857/#review3199

> I'm sure this isn't needed, but I couldn't figure out how to access the POST method from RBClient without it, since root.get_path() fails unless there's a GET handler.  I wanted to post this for review nonetheless, so just let me know what I'm doing wrong. :)

Ya, that's a deficiency of Review Board's API + how RBTools builds it's objects. Since RBTools needs to find a link with a POST method to allow create, the resource must be fetched first (It'd be great if RB provided all these specifications for what HTTP methods a resource takes in the root).

So, sadly this *is* the correct way for now :/

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review3239

This works for me, though you'll probably want a second pass from smacleod. Good work on this.

::: pylib/mozreview/mozreview/extension.py
(Diff revision 5)
> +from resources.restrict_review_requests import restrict_review_requests_resource

I think we've been using the entire package path, so:

`from mozreview.resources.restrict_review_requests import restrict_review_requests_resource`

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 5)
> +from djblets.webapi.decorators import (webapi_login_required,

New files should get the from __future__ unicode literals thing

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 5)
> +                extra_data__contains=identifier_prefix):

I didn't realize we put the whole bz://id/ into the children's extra data. Interesting.

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 5)
> +    def unmake_admin(self, path, email):
> +        import sqlite3
> +        db = os.path.join(path, 'reviewboard.db')
> +        conn = sqlite3.connect(db)
> +        with conn:
> +            conn.execute('UPDATE auth_user SET is_superuser=0, is_staff=0 '
> +                    'WHERE email=?', (email,))
> +            conn.commit()

Bodacious

Comment 23

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Web API endpoint to restrict review-request visibility.

Pull down this commit:

hg pull review -r ae658989867ee945160098a62c0d33a44ba49ae9

Comment 24

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Web API endpoint to restrict review-request visibility.

Pull down this commit:

hg pull review -r 9cd38df760d2790b4fa739d058d297d7cb92f185

Comment 25

[Mark Côté [:mcote]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

/r/2859 - Bug 1113208 - Web API endpoint to restrict review-request visibility.

Pull down this commit:

hg pull review -r e1e27eef55d616c12957ab45162a3d53f3142374

Comment 26

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/2859/#review3499

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 8)
> +    rr.target_groups.add(Group.objects.get(name=confidential_group_name))

Instead of passing the group name, lets pass the actual group object itself.

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 8)
> +        if not Group.objects.get(name=confidential_group_name):

Save this group to a variable and pass that into restrict_review_request instead of the name

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 8)
> +            return SERVICE_NOT_CONFIGURED.with_message(
> +                'group %s does not exist' % confidential_group_name)

I feel like we should be doing permission checking before spitting out these configuration errors.

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 8)
> +        for rr in ReviewRequest.objects.filter(
> +                extra_data__contains=identifier_prefix):

This is going to be very expensive as the number of review requests grow. We should know which review identifier to find directly (it's in the commit id field and is indexed) or even the parent review requests id.

Above won't cover cases where everything is in an initial draft, and bugzilla hasn't heard about it yet though... Those drafts won't be allowed to publish since the bug has become confidential, but we won't find the pushed commits in order to clear them on the hg side....

Not really sure what we should do. I guess I could live with this query for now, it will be rarely executed...

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 8)
> +        for rrd in ReviewRequestDraft.objects.filter(
> +                extra_data__contains=identifier_prefix):

ya, two of them, this is going to be SLOOOW.

::: pylib/mozreview/mozreview/resources/restrict_review_requests.py
(Diff revision 8)
> +    def post(self, request, bug_id, *args, **kwargs):

We should add some logging into this function, especially if it somehow 500s or fails. Any error that stops it from completing operations will cause things to be left in the open. Useful logging would indicate which portion of this failed.

Also, it'd be nice if we could notify someone if it appears in the logs as failing. Don't we have some Mozilla system consuming data from our logs? could that be used?

Either way, just toss in a little logging before landing and notifications and whatnot could be dealt with later.

Comment 27

[Steven MacLeod [:smacleod]]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review3501

Fix the issues brought up in my commit review, then Ship It!

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8553317 [details]
MozReview Request: bz://1113208/mcote

https://reviewboard.mozilla.org/r/2857/#review4023

Ship It!

Comment 29

[Mark Côté [:mcote]]

We're not going to try to go through all this headache after all; we just need to remind people in different ways that MozReview is a fully public system.