Validate that a UI-Auth session is not being reused

[clokep]

This is my attempt to fix:

make sure that UI-Auth sessions are tied to a single request (ie, the client can't change its mind about what it's asking for in the middle of a session)

The general idea is to store some additional information in the session about what is being authenticated (namely the URI, the method, and the data associated with whatever operation is being performed). So at each stage in when we check the status of the user interactive authentication session we ensure that the event being authenticated for has not been modified.

This isn't fully finished, but I wanted to put this up and get some feedback.

Part of #5667

TODO

• Finalize design.
• Fix unit tests.

Currently based on #7063.

[clokep]

matrix-org/sytest#830 starts adding some tests for this.

[clokep]

I'm requesting review on this to get some early feedback. There's a bunch of TODOs in the code still, but want to make sure the approach seems reasonable before finishing this up.

[richvdh]

the general approach seems sensible to me.

It's not obvious to me what your intention for action_id is, and it's not obvious to me that a 2-tuple (action_type, action_id) is always going to be the best way to distinguish distinct operations. How about doing one of the following:

• have the caller pass in a dict structure describing the operation in a format applicable to the operation in question. For example for a device deletion, this could be { "operation": "delete_device", "device_id": "<foo>" }
• have the caller do something like that, but then calculate a hash and pass that into validate_user_via_ui_auth
• I wonder if we can take this completely out of the REST layer's hands, and jjust calculate a hash based on body (minus auth) and the request method and uri.

[clokep]

It's not obvious to me what your intention for action_id is, and it's not obvious to me that a 2-tuple (action_type, action_id) is always going to be the best way to distinguish distinct operations. How about doing one of the following:

The idea was that there's a variety of actions that need to use UI Auth (deleting a device, adding a 3PID, etc.) this would get stored as the "action type" and the "action ID" would be whatever the action is being taken on (the user or the device or something). The TODO comments show that there isn't always a clear item to put here. I believe when I made that realization I decided to get feedback on the design!

I like the idea of leaving the format completely up to the caller (so it becomes opaque to the session code). I believe I tried to make it so that the rest code didn't need to know anything about it, but I ran into issues. I'll re-evaluate that again and see if I can get it to work.

[clokep]

I wonder if we can take this completely out of the REST layer's hands, and jjust calculate a hash based on body (minus auth) and the request method and uri.

I looked into this a bit again, we might be able to do this by passing through the request to the handlers. I'll give this a try.

[clokep]

Without this change this test fails the checks for modifying the operation during a UI Auth session. I think this is expected and updating the test is the right way to go here.

[clokep]

@richvdh I think this is ready for a review again. I think this is in better shape now, although I'm a little bit worried there might be break clients...but maybe that's kind of the point if they're doing something insecure?

[richvdh]

lgtm otherwise

[richvdh]

lgtm