Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
njouanin opened this issue Feb 7, 2019 · 9 comments
Open

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

njouanin opened this issue Feb 7, 2019 · 9 comments
Labels
A-Authentication A-Registration O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Z-Spec-Compliance An area where Element doesn't correctly implement the spec

Comments

@njouanin
Copy link

njouanin commented Feb 7, 2019

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

  • get riot-web registration form.
    • riot-web makes a first call to register with {"auth":{}}.
    • Plasma replies with {"completed":[],"flows":[{"stages":["m.login.email.identity"]}],"params":{},"session":"Mzg3NWRlMzUtM2VhYS00ZjNhLWJjMmYtYzFiMDM2ZDIyMjEz"}
  • fill and submit registration form:
    • riot-web posts to register endpoint with {"auth":{},"username":"xxxx","password":"xxxxxx","bind_email":true,"bind_msisdn":true,"x_show_msisdn":true}
    • because auth request doesn't contain the previous session attribute Plasma ignores the request and replies with a new session {"completed":[],"flows":[{"stages":["m.login.email.identity"]}],"params":{},"session":"NDEzOThlZDQtMmYxNS00OTFlLWJkZDAtMWI5MmMzYjEzNTNi"}

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

Version information

  • riot-web v0.17.9
  • Firefox / Linux
  • Plasma (unreleased)
@jryans jryans changed the title rio-web doesn't preserve session id returned by server auth API riot-web doesn't preserve session id returned by server auth API Feb 8, 2019
@jryans jryans added T-Defect X-Cannot-Reproduce S-Major Severely degrades major functionality or product features, with no satisfactory workaround A-Registration defect P2 and removed X-Cannot-Reproduce labels Feb 8, 2019
@njouanin
Copy link
Author

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

@turt2live turt2live self-assigned this May 2, 2019
@turt2live turt2live added the Z-Spec-Compliance An area where Element doesn't correctly implement the spec label May 2, 2019
@joepie91
Copy link

joepie91 commented May 2, 2019

@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-spec-proposals#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
Copy link

joepie91 commented May 5, 2019

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
Copy link
Author

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
Copy link
Member

t3chguy commented Jun 14, 2020

@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
Copy link
Author

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 :

  • a first call to /register with empty auth when the registration form is first displayed => mascarene replies 401 with a session Id
  • a second call to /register when the form is submitted. The request contains registration parameters (login, password), but the previous session Id is not set => mascarene replies 401 with a session Id
  • a third call to /register is then sent back with registration info AND session id => mascarene processes successfully.

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

@t3chguy
Copy link
Member

t3chguy commented Jun 14, 2020

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

@njouanin
Copy link
Author

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

@njouanin
Copy link
Author

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 ?

@jryans jryans removed the defect label Mar 4, 2021
@turt2live turt2live added S-Minor Impairs non-critical functionality or suitable workarounds exist O-Uncommon Most users are unlikely to come across this or unexpected workflow and removed P2 S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Authentication A-Registration O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Z-Spec-Compliance An area where Element doesn't correctly implement the spec
Projects
None yet
Development

No branches or pull requests

5 participants