1340590 - The UI always blames API 403s on not being logged in when there can be other causes

Status: NEW

(Tree Management :: Treeherder: Frontend, defect, P3)

Description

[Jim Mathies [:jimm]]

I'm currently logged in to Treeherder through Okta. When I request retriggers I get:

"Please login to Treeherder to complete this action"

Yet I'm clearly logged in according to the UI.

Comment 1

[Ed Morley [:emorley]]

If you load Treeherder in a new tab, does the login state still show as logged in? (Currently login expiring is handled poorly in some cases)

Failing that, can you capture the request for the retrigger that doesn't return HTTP 200 please.

Comment 2

[Jim Mathies [:jimm]]

Attachment: content.jpg

I just hit the same page and it shows me as logged in. attached.

Comment 3

[Jim Mathies [:jimm]]

Attachment: netdbg window.jpg

request, 403.

Comment 4

[Jim Mathies [:jimm]]

Thought that HTTPS Everywhere might be the problem, but disabling that doesn't help.

Comment 5

[William Lachance (:wlach)]

So the only way you could get a 403 is (I think) if Treeherder thought you weren't authenticated:

https://github.com/mozilla/treeherder/blob/master/treeherder/webapp/api/jobs.py#L420

Looking at papertrail, I see lots of examples if this operation succeeding, so I suspect some kind of intermittent issue with your session. I think I see the log line with your failure, unfortunately there is no helpful context. :( https://papertrailapp.com/systems/treeherder-prod/events?selected=769323282196504660&focus=769323282196504660

This is about the limit of my knowledge of treeherder authentication. In addition to :emorley, :camd might be able to help.

Comment 6

[Cameron Dawson [:camd]]

Jim- If you log out and back-in, does that remedy this?  I wonder if we are hitting an issue with session expiry not matching the localstorage item we have stored.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Cameron Dawson [:camd] from comment #6)
> Jim- If you log out and back-in, does that remedy this?  I wonder if we are
> hitting an issue with session expiry not matching the localstorage item we
> have stored.

Nope. I also tried a fresh login from a container tab type I normally don't use, still same error.

Comment 8

[Ed Morley [:emorley]]

Have you changed referrer settings or use a referrer privacy addon in this profile? (Django uses referrer in CSRF checks to prevent certain attack vectors.)

Could you try with a clean profile?

Comment 9

[Ed Morley [:emorley]]

Ah so I can reproduce symptoms which at least look similar to yours, via:

1) In about:config set `network.http.sendRefererHeader` to `0` (default is `2`, see http://kb.mozillazine.org/Network.http.sendRefererHeader)
2) Visit https://treeherder.mozilla.org/
3) Log in
4) Select a completed job (in my case I selected the same buildbot win7 vm bc3 job type)
5) Press retrigger
6) Inspect response in console

Expected:
* Retrigger succeeds, or else an actionable error message shown.

Actual:
* Error message reads "Please login to Treeherder to complete this action"
* The XHR request to /api/project/mozilla-inbound/jobs/retrigger/ 403s, and the response body was `{'detail': 'CSRF Failed: Referer checking failed - no Referer.'}

As such there are two parts to this:
1) The error message hiding the true cause, since it doesn't make use of the `detail` attribute in the response
2) Retriggers not working when the referrer is not set

We should definitely fix #1.

However #2 is bug 1263731, which is wontfix since:
* Django makes a seemingly valid case for why these referrer checks are required (https://github.com/django/django/blob/1.10.5/django/middleware/csrf.py#L196-L210), and wontfixed making the check less strict (https://code.djangoproject.com/ticket/16870).
* Even if someone can propose an alternative implementation that mitigates the attack the checks are trying to prevent, forking the Django CSRF middleware adds risk/maintenance burden, so I'd want this improvement to land upsteam in Django first.
* Very few people disable referrers, and for those that wish to do so there is a workaround, which is to use an addon that provides more nuanced referrer behaviour: https://addons.mozilla.org/en-US/firefox/addon/smart-referer/

Pre-emptively morphing the bug to be about #1, though please do confirm if this was the cause. (If not, providing the body of the 403 response would really speed things up here.)

To fix this we need to change:
https://github.com/mozilla/treeherder/blob/98f7d3fe97079218cf156029cb02c0963c41613e/ui/js/models/error.js#L22-L24

Comment 10

[Jim Mathies [:jimm]]

Resetting 'network.http.sendRefererHeader' fixes the PROBLEM. Thanks!

Comment 11

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #10)
> Resetting 'network.http.sendRefererHeader' fixes the PROBLEM. Thanks!

oops, unintentional caps.

Comment 12

[Ed Morley [:emorley]]

Thank you for confirming :-)

Comment 13

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:1340590 > mozilla:master

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8841163 [details] [review]
[treeherder] KWierso:1340590 > mozilla:master

Would this work?

Comment 15

[Ed Morley [:emorley]]

Comment on attachment 8841163 [details] [review]
[treeherder] KWierso:1340590 > mozilla:master

Thank you for taking a look at this :-)

I'd like to avoid hardcoding a handful of error strings - there are many more CSRF strings Django uses for other cases for example. I think the best way forwards with this bug is to see what the error message is for the more common cases (eg not signed in. To simulate this have two tabs open, sign out in the 2nd then try to retrigger in the first) and see if the conditional can be inverted to check for the absence of those.

Comment 16

[Ed Morley [:emorley]]

Hmm the more I think about this, the more I think the best solution is just to fix bug 1318092, then we don't have to hardcode any strings at all.

ie something like:

if HTTP401/403...
  <make request to /api/user/ via ThUserModel>
  if still signed in...
    <show `detail` string from API response which should help debug>
  else...
    <update login state to "signed out">
    <show existing simpler "please sign in" message>