1149176 - Errors from expired sessions don't bubble up to prevent the action

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️) (Catching up on needinfos)]

jimm gave me a review on a child commit here:

https://reviewboard.mozilla.org/r/6163/

I expected that review to have its comments mirrored to the bug (bug 1148012).

But no mirroring occurred.

Comment 1

[Mark Côté [:mcote]]

Aha, I see this in the log.  It *is* due to expired tokens.  Gr.  We should fix this to actually stop the publishing from happening; even with API keys, we want to detect when one has been invalidated.

14:53:45 	ERROR 	

 - Error when calling <function on_review_publishing at 0x7f008f16db18> from SignalHook: Publish error: Bugzilla error: The cookies or token provide were not valid or have expired. You may login again to get new cookies or a new token.
Traceback (most recent call last):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/Djblets-0.8.15-py2.6.egg/djblets/extensions/hooks.py", line 196, in _wrap_callback
    self.callback(extension=self.extension, **kwargs)
  File "build/bdist.linux-x86_64/egg/rbbz/extension.py", line 180, in _transform_errors
    raise PublishError('Bugzilla error: %s' % e.msg)
PublishError: Publish error: Bugzilla error: The cookies or token provide were not valid or have expired. You may login again to get new cookies or a new token.

Comment 2

[Mike Conley (:mconley) (:⚙️) (Catching up on needinfos)]

Is the workaround for this to log out of Review Board and log back in?

Comment 3

[Mark Côté [:mcote]]

Yes.

Comment 5

[Mark Côté [:mcote]]

Better summary of the problem.  We also have a very easy fix! \o/

Comment 6

[Mark Côté [:mcote]]

Attachment: MozReview Request: bz://1149176/mcote

/r/7009 - Bug 1149176 - Don't sandbox errors in hooks that talk to Bugzilla. r=smacleod

Pull down this commit:

hg pull -r 4bb3af0573898f0b2e2810466c3f418bf80d89dd https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7009/#review5817

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
>          SignalHook(self, review_request_closed,
> -                   on_review_request_closed_discarded)
> +                   on_review_request_closed_discarded,
> +                   sandbox_errors=False)

I don't think this is an abortable signal, we want the sandboxing here.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +        # Any signal hooks that talk to Bugzilla should have
> +        # sandbox_errors=False, since we don't want to complete the action if

any "abortable" signal maybe?

Comment 8

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/7009/#review5821

> I don't think this is an abortable signal, we want the sandboxing here.

Ah shoot, you're right.  I think we *should* be using an abortable signal here, but that's a separate bug.

Comment 9

[Mark Côté [:mcote]]

Comment on attachment 8591848 [details]
MozReview Request: bz://1149176/mcote

/r/7009 - Bug 1149176 - Don't sandbox errors in hooks that talk to Bugzilla. r=smacleod

Pull down this commit:

hg pull -r a58f73fba8c2a11d10446f8cbc71603d391148df https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 10

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7009/#review5919

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

Get rid of this blank line.

Comment 11

[Steven MacLeod [:smacleod]]

Comment on attachment 8591848 [details]
MozReview Request: bz://1149176/mcote

https://reviewboard.mozilla.org/r/7007/#review5921

Fix it, then Ship-it!

Comment 12

[Mark Côté [:mcote]]

http://hg.mozilla.org/hgcustom/version-control-tools/rev/a6dc54687ced

This should be deployed to production early next week, hopefully on Monday.

Comment 13

[Mike Conley (:mconley) (:⚙️) (Catching up on needinfos)]

Is there a bug on file to find some way of refreshing the token to avoid this problem entirely?

Comment 14

[Mark Côté [:mcote]]

Not really, since you have to log in with your password, and we don't store that.

However, we will be switching to API keys once bug 1144468 is fixed to allow easy authorization of MozReview, and those won't expire (unless the user explicitly invalidates it).  So that should solve the problem properly.

Comment 16

[Mark Côté [:mcote]]

Attachment: MozReview Request: Bug 1149176 - Don't sandbox errors in hooks that talk to Bugzilla. r=smacleod