element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.05k stars 1.97k forks source link

riot-web doesn't preserve session id returned by server auth API #8458

Open njouanin opened 5 years ago

njouanin commented 5 years ago

Description

riot-web seems to not preserve the session id returned by the server auth API first call. Because this session is not sent in next API calls, the server ignore the auth call and replies with a new session. Synapse seems to ignore this, but Plasma does not.

See §5.3.2 of client-server-spec: This is a session identifier that the client must pass back to the home server, if one is provided, in subsequent attempts to authenticate in the same API call.

Steps to reproduce

In the previous example, riot-web request should have contained : "auth":{"session": "NDEzOThlZDQtMmYxNS00OTFlLWJkZDAtMWI5MmMzYjEzNTNi"} along with registration data.

Version information

njouanin commented 5 years ago

Tested again with riot-web 1.0.8 and this issue is still there.

joepie91 commented 5 years ago

@turt2live suggested that I pitch in here with a related issue... per the spec, regarding the auth property on the registration endpoint:

It should be left empty, or omitted, unless an earlier call returned an response with status code 401.

... however, Riot immediately fires a POST request at that route with the following body:

{ "auth": {} }

... despite my HS never returning a 401 at any point in time (as User-Interactive Authentication is not yet implemented at all).

The specification is admittedly a bit vague in what exactly "empty" means, but I'd interpret it to mean that the client shouldn't attempt UIA at all unless told to do so by the HS, and thus this would be a violation of that.

Related: matrix-org/matrix-doc#1980


Edit: The section on User-Interactive Authentication is actually much clearer on this, and confirms that Riot does indeed violate the specification:

A client should first make a request with no auth parameter [1]. The homeserver returns an HTTP 401 response, with a JSON body, as follows:

joepie91 commented 5 years ago

I cannot reproduce the original issue with User-Interactive Authentication on the register endpoint. In my case, Riot is correctly persisting the ID.

Debug output from my HS implementation, snipped for brevity:

## POST /_matrix/client/r0/register
{ query: {},
  params: {},
  host: 'localhost',
  headers: 
   [... snipped ...]
  cookies: undefined,
  body: 
   { auth: {},
     username: 'joepie91',
     password: 'testtest',
     bind_email: true,
     bind_msisdn: true,
     x_show_msisdn: true } }
Unhandled error: UIARequired: User-Interactive Authentication is required for this endpoint
[... snipped ...]
  errorMeta: 
   { session: '1AGiKTE0eyTMbKRiLjXqH',
     completed: [],
     params: { m: undefined },
     flows: [ { stages: [ 'm.login.dummy' ] } ] } }

## POST /_matrix/client/r0/register
{ query: {},
  params: {},
  host: 'localhost',
  headers: 
   [... snipped ...]
  cookies: undefined,
  body: 
   { auth: { session: '1AGiKTE0eyTMbKRiLjXqH', type: 'm.login.dummy' },
     username: 'joepie91',
     password: 'testtest',
     bind_email: true,
     bind_msisdn: true,
     x_show_msisdn: true } }

The errorMeta contains the extra properties sent to Riot in the initial 401 response, including the session ID. In the body of the second POST request, that same session ID is present.

@njouanin In your output, I can see that both POST requests include an empty auth object in the request body; the second request should have included at least a { "type": "m.login.email.identity" } key.

This suggests that perhaps Riot did not understand the first response to be an UIA response at all (did that response correctly carry a 401 status code?), or it is not familiar with the m.login.email.identity stage and therefore bugs out with an 'empty' authentication attempt?

Can you try to reproduce with an m.login.dummy stage instead, and see if the issue persists?

njouanin commented 5 years ago

Tested again wit riot-web 1.1.2. Using m.login.dummy stage riot-web provides back the session id when posting the second /register call and uses the correct workflow.

t3chguy commented 4 years ago

@njouanin there have been recent changes in this area to make it compatible with the Conduit rs homeserver (WIP) - could you please check if it still affects Plasma?

njouanin commented 4 years ago

Is there a riot release I can use to test it ? I'm currently using riot 1.6.0 for Mascarene developement, and it the workflow seems to work: What I currently see is :

So, it works but may be riot should use the first session ID, so we can spare 1 API call.

t3chguy commented 4 years ago

Try riot.im/develop for the latest but the vast majority of the changes are already in Riot 1.6.4

njouanin commented 4 years ago

just tested with 1.6.4 can't see difference in the registration workflow. Still working with mascarene.

njouanin commented 4 years ago

besides, I have a regression when sending a message to a room. the tying API is not implemented currently on mascarene, so it return 404 and it seems to prevent riot from sending the message into the room. This was working on 1.6.0. Should I open a new bug ?