Save login tokens in database

[sandhose]

Fixes #13841, based on #13842

One important thing this does, is that now login tokens can only be used once. Before, they could be used multiple times before they expired.

[sandhose]

This is not needed, because the check_auth_blocking is called by the caller. So before we effectively checked that twice.

[sandhose]

This PR is now ready for reviews, but first needs #13842, because it completely removes the ModuleApi.generate_short_term_login_token method.

[richvdh]

One important thing this does, is that now login tokens can only be used once.

This seems like it might be problematic. If the response from the /login request gets lost (eg, the network connection drops), then shouldn't the client be able to retry?

[sandhose]

One important thing this does, is that now login tokens can only be used once.

This seems like it might be problematic. If the response from the /login request gets lost (eg, the network connection drops), then shouldn't the client be able to retry?

Not sure if it is worse than letting a login token issue many access tokens. For reference, in OAuth, they forbid authorisation code reuse, and suggest re-initiating an auth flow, since the user is likely to already be connected, and it should be quick enough.

Section 4.1.2 of the OAuth 2.0 spec:

The authorization code MUST expire shortly after it is issued to mitigate the risk of leaks. A maximum authorization code lifetime of 10 minutes is RECOMMENDED. The client MUST NOT use the authorization code more than once. If an authorization code is used more than once, the authorization server MUST deny the request and SHOULD revoke (when possible) all tokens previously issued based on that authorization code.

[richvdh]

Not sure if it is worse than letting a login token issue many access tokens.

Well, in an ideal world we'd invalidate the first access token; we'd also invalidate the login token as soon as the access token it created was used. But that's not easy.

For reference, in OAuth, they forbid authorisation code reuse, and suggest re-initiating an auth flow, since the user is likely to already be connected, and it should be quick enough.

Interesting. It's not about "quick enough", and more about the user experience, which is that if a login token fails, the user is just given some "computer says no" page.

Anyway, fair enough. I guess if OAuth does it, it must be acceptable, though it goes against my instincts.

[erikjohnston]

How does this work with idempotency? Ideally we'd be able to retry the login request. Otherwise on awful networks you might get stuck trying to login, fail to get the response, have to get another login token, etc.

[sandhose]

We had this discussion with VdH in the comments here: #13844 (comment)

OAuth suggests only accepting an authorisation code once, even if the response gets lost, and instead re-start an authorisation.
It is worth noting that Element Web won't retry exchanging the login token if the request fails. I haven't checked what other clients do.
It is, in my opinion, better than letting a single token issue an infinite number of sessions, especially since there is nothing in m.login.sso verifying who is exchanging the token

[erikjohnston]

Oh, sorry.

I agree that we shouldn't allow infinite uses, but there are standard ways to avoid that, by e.g.:

• having client send a txn ID and then only allowing login token to be used with that txn ID going forwards, etc. Though in this case we don't have one for /login API (though we probably should).
• record the access token returned, and delete from login_tokens once we see it used.

I'm a bit in two minds about whether this is fine to leave as is for now. On the one hand its definitely something we could add later, but on the other hand its also something that is only going to bite people on bad connections, which is something we rarely suffer from ourselves and so are unlikely to notice how problematic it is.

[sandhose]

Maybe we could start tracking that by recording token reuse in a prometheus counter?

[sandhose]

@erikjohnston: I added a Prometheus counter (fb0b2f2) to track invalid login token, so we know if a token was reused, expired or if it is just unknown

[erikjohnston]

Ok, let's leave this as is for now. Though I would love it if when we came to changing the /login API we added a session ID to allow us to get out of this situation.