Explicitly create UI Auth sessions

[clokep]

This makes a tweak to the way that UI Auth sessions get created that I think will be necessary to persist UI Auth sessions (for #6705). UI auth sessions are now created explicitly when no session ID is provided (i.e. at the beginning of a UI auth negotiation).

This does have a change of behavior that clients might notice, but I think being stricter here avoids an abuse channel. Instead of lazily creating sessions if the UI Auth session is unknown (e.g. if the session ID is not found) this will now error with an unknown session.

As far as I can tell the result is unspecced when an unknown session ID is provided, so the previous behavior was being generous, but does not need to be supported.

[anoadragon453]

Although the previous behaviour was unspecified, it'd still be good to check that it won't break any client flows.

And if so, give people heads up before we do so ala #6766

[anoadragon453]

This looks mostly good to me, but I feel I need to give it a once over tomorrow morning when it's not so late here just to make sure I didn't miss anything.

[clokep]

Although the previous behaviour was unspecified, it'd still be good to check that it won't break any client flows.

And if so, give people heads up before we do so ala #6766

I need to think a bit about this too. Will try to get some answers!

[anoadragon453]

I might be missing it, but I'm not sure where this code now rejects unknown sessions instead of creating them silently?

[clokep]

I might be missing it, but I'm not sure where this code now rejects unknown sessions instead of creating them silently?

@anoadragon453 It was a bit obtuse how it could happen, hopefully the following provides an example of what could happen:

An easy path to follow to get to that code is:

• The body from a password reset request is passed into check_auth.
• check_auth pulls out the auth dict from the body, and checks if for a session key.
• check_auth calls _get_session_info with the given session ID.
• _get_session_info ignores the session ID if it is unknown and generates a new one.

I think there are other code paths that could happen too, but I'm not sure.

[clokep]

but I think being stricter here avoids an abuse channel.

After thinking on this statement more I don't think this is true. Before or after this change clients can start arbitrary sessions with the server. Additionally, the endpoints are rate limited so I think this is OK.

[clokep]

As far as I can tell the result is unspecced when an unknown session ID is provided, so the previous behavior was being generous, but does not need to be supported.

The specification does say a little bit that applies here, but nothing specific to this case:

If the request fails for a reason other than authentication, the server returns an error message in the standard format. For example:

HTTP/1.1 400 Bad request
Content-Type: application/json

{
  "errcode": "M_EXAMPLE_ERROR",
  "error": "Something was wrong"
}

It is also possible that instead of returning an error we could:

• Create a new session
• Return a 401 error as the session is new (e.g. no phases have been complete, etc.)

But I'm not sure if clients check the session ID that is returned on every response. Returning an error and bailing seems more reasonable to me.

[clokep]

Although the previous behaviour was unspecified, it'd still be good to check that it won't break any client flows.

I was able to test this with Riot web by doing the following:

• Login with an account
• Go to the settings and delete a device
• (You should see a password prompt come up, don't fill it in yet)
• Restart the Synapse server (really what we're trying to do here is flush the cache of activate UI auth sessions)
• Type in the password and continue the UI Auth session

On our current develop this works and the authentication is completed.

The changes in this branch make an error pop up that says "Unknown session ID: " (Riot seems to just pass through the error from Synapse).

Obviously the user experience on develop is better, but I'm not sure it will work properly if there are more than a single stage in the UI auth flow.

Note that this example of restarting synapse is a bug, the same bug that I aim to eventually fix (#6705), this is just a way to invalidating a session ID.

[anoadragon453]

Obviously the user experience on develop is better, but I'm not sure it will work properly if there are more than a single stage in the UI auth flow.

Yeah, if you've already completed one stage, then the server restarts, you'll need to complete that stage again.

And it may be better right now, but #7302 will be the best of both worlds!

Thanks for the detailed explanation, and as of 6fa823c I now see where unknown sids are rejected.

If anything this goes by the mantra of "if something fails, have it fail loudly than silently".

[anoadragon453]

lgtm other than the below and above.

I'm not entirely convinced this won't break some random client, but seeing as there isn't an easy way to scan the DB for it and they're not spec compliant, I think that's fine.

Though it may be worth testing with some popular mobile clients just to be sure.