1453837 - "TypeError: '<' not supported between instances of 'str' and 'int'" in auth backend under Python 3

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P1)

Description

[Ed Morley [:emorley]]

Running this (see https://docs.python.org/2/using/cmdline.html#cmdoption-3):
`python -3 -m pytest tests/webapp/api/test_auth.py`

Gives:
```
treeherder/auth/backends.py:140: DeprecationWarning: comparing unequal types not supported in 3.x
    session_expiry_in_ms = min(accesstoken_exp_in_ms, idtoken_exp_in_ms)
```

(Note I'll soon by suppressing the DeprecationWarning in the pytest setup.cfg config, so that will need commenting out first to see it.)

Investigating, this is a real bug that is affecting us even under Python 2.

Currently the `min()` call will always return `accesstoken_exp_in_ms` (which is an int), and never `idtoken_exp_in_ms` (which is still a string). (Since when comparing a string and int in Python 2, it always returns the int.)

When I changed:
    idtoken_exp_in_ms = user_info['exp'] * 1000

...to:
    idtoken_exp_in_ms = int(user_info['exp']) * 1000

The test started failing with an HTTP 400, and the following log output:
[2018-04-12 20:52:48,955] WARNING [treeherder.auth.backends:143] Updating session to expire in -1523565868 seconds
[2018-04-12 20:52:48,960] ERROR [django.security.SuspiciousOperation:80] The request's session was deleted before the request completed. The user may have logged out in a concurrent request, for example.

So it seems that:
1) We're currently never using the ID token expiry at all, even when it was lower
2) We're missing test coverage for ID token
3) The tests we have already might be using incorrect mocked data or else there is a 2nd bug in the ID token expiry handling that is causing the HTTP 400 and `-1523565868 seconds` (for example it looks like we might be confusing relative time deltas with timestamps)

Comment 1

[Ed Morley [:emorley]]

I found several more bugs whilst looking into this:

• the tests currently use the wrong value for id token's exp (they use a value like 500 when it should be a unix timestamp)
• the Django session length is only set when fetching an existing user, and not in the case of creating a new user
• the presence of the id token header isn't validated
• several permuatations of invalid id token result in HTTP 500 rather than a 403
• already expired expiration times can result in a negative Django session length rather than a useful error
• test coverage is low

I've almost finished a branch that addresses these. Hopefully with that complete, we should be able to start trying out Python 3 on prototype with the aim of landing it soon.

Comment 2

[Ed Morley [:emorley]]

Morphing this to be about just fixing the failures under Python 3, since the scope of fixing the other auth backend issues has increased so worth splitting them out to a separate bug.

Comment 3

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/4667

Comment 4

[Ed Morley [:emorley]]

https://github.com/mozilla/treeherder/commit/ecabd76f9604c6dcd0a11389949975c3e355a7c4