UIAA for register is wrong (needs MSC)

[timokoesters]

Registering on Conduit fails. I looked at the request jsons and it looks like it sends a /register request with just auth and session keys. The spec says a client should retry the original request PLUS auth and session.
https://matrix.org/docs/spec/client_server/r0.6.1#user-interactive-api-in-the-rest-api

[bmarty]

Signup flow on Element Android is documented here.

Is there something wrong in the documentation, or do you think the current implementation is not conform to the documentation?

FWIW, we have no problem when creating on account on Synapse, which I admit does not guarantee that there is no bug though.

[timokoesters]

The spec has required fields in /register, but the 'First step' request body is empty, so conduit returns 'bad request' and makes element android say the server does not allow registration

[bmarty]

I do not see any required fields for the /register endpoint (I've looked at https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-register).

Also I do not see the link between your last comment and the issue description.

[timokoesters]

Okay, ignore my last comment, what's important is this:
Element Android's register request to the server is this:

{"username":"hbddd","password":"hdhdbs","initial_device_display_name":"Element Android"}

Conduit responds with the uiaa info containing a
Then it sends another request:

{"auth":{"type":"m.login.dummy","session":"<sessiontoken>"}}

Conduit responds with "Forbidden" because it assumes the request contains all the fields from the first request

[timokoesters]

Element Web does this:

1. A /register request like this:

{"initial_device_display_name":"app.element.io (Firefox, Linux)"}

(It seems to not care about the uiaa token of the response, so maybe it does this to check if registration is enabled?)

2. Another /register, this time with all the important fields

{"username":"slfj","password":"asf","initial_device_display_name":"app.element.io (Firefox, Linux)","inhibit_login":false}

3. One last /register request using the UIAA token from the response of 2

{"username":"slfj","password":"asf","initial_device_display_name":"app.element.io (Firefox, Linux)","auth":{"session":"<sessiontoken>","type":"m.login.dummy"},"inhibit_login":false}

[bmarty]

Ok, I got it.
So to my point of view, If the client sends the username and password to the homeserver and get a UIAA token back, the client expects that the homeserver stores the previously sent data with UIAA as a key. This is already the case for the completed stages for instance.
I know that Riot-Android was always sending the password and username with each /register requests, and apparently Element Web does the same, but to me it is not correct, and maybe a security hole because the app will have to remember the username and the password of the user for the duration of the registration.
When implementing it on Element Android, I was happy to see that username and password are actually stored by Synapse during the whole registration process, so there was no use to send them for each /register request.

In your case, even for Element-Web, the second third request is not the same request, but is a request to perform the stage m.login.dummy.

So I think UIAA for registration is a bit particular regarding the documentation https://matrix.org/docs/spec/client_server/r0.6.1#user-interactive-api-in-the-rest-api

Do you agree with my point of view?

[timokoesters]

Currently UIAA tokens are only valid for the user who created them. That means I need to know the user id to check if a uiaa token is valid, which is not the case for /register without the username field. Secondly, the way conduit endpoints work (using the ruma library to deserialize them) makes sure that required fields are present and otherwise automatically returns "Bad Request" to them. This doesn't work with the way you want UIAA to work and hard to change

[bmarty]

Ok, I will check internally what is the decision to take here, thanks for your patience :).

I understand that your homeserver (with ruma) expects mandatory fields and this is good to have request validation server side :).

Also, what is you plan to tell the client if registration is enabled or not? As discussed above, Element Android sends an empty body and Element Web sends only the initial_device_display_name. So the expected fields are also not there.

[bmarty]

FTR I also see in one unit test from Synapse that the above mentioned fields are not always sent, which confirm that this request is well valid for Synapse.

[anoadragon453]

FTR I also see in one unit test from Synapse that the above mentioned fields are not always sent, which confirm that this request is well valid for Synapse.

I'm afraid Sytest does indeed test whether requests are valid for Synapse - but this may not translate to being valid as defined by the spec :)

@timokoesters is arguing the spec states that in order for a client to choose and complete its first chosen stage, "A client should first make a request with no auth parameter. ... The client then chooses a flow and attempts to complete the first stage. It does this by resubmitting the same request with the addition of an auth key in the object that it submits" (emphasis mine).

From the examples @timokoesters posted, it looks like Element Android is trying to complete a stage without including the keys from the previous request, which is technically against the spec.

I believe the intention of sending the keys each time is so that the server has less book-keeping to deal with. Synapse, being lax from the beginning, did not want to break old clients, and thus indeed stores all information about a register request as it learns it (however is careful to store passwords hashed).

Fundamentally this is an instance of the spec being a bit vague. For instance, technically Element Android doesn't have to send the initial keys during its attempt at completing a second stage, as the spec only explicitly requires it being necessary for the first - even though this doesn't make any logical sense.

I'm rather happy to see new server/client implementations following the spec to the letter and learning the pitfalls of the current iteration. As the spec currently stands @timokoesters is right and Element Android should resend the keys. If that's not possible or extremely difficult for Element Android to do, then we can make a bid to update the spec, although it should certainly be updated regardless to help settle misinterpretation in this area.

I believe it's fine if older versions of Element Android won't work here.

[clokep]

FTR I also see in one unit test from Synapse that the above mentioned fields are not always sent, which confirm that this request is well valid for Synapse.

I'm afraid Sytest does indeed test whether requests are valid for Synapse - but this may not translate to being valid as defined by the spec :)

The UI auth unit tests are more interesting here then the initially given one. There are also two interesting sytests:

1. For for being idempotent on registration.
2. For remembering parameters between calls.

I believe that according to the specification clients are supposed to provide the non-auth parameters on each request, but Synapse has backwards compatibility features to allow clients to not do this. The relevant logic in Synapse, that code has been modified recently, but I can't say that the ideas behind the comments are all still valid or not.

I do think the spec needs to be clarified here and made explicit one way or the other.

[KitsuneRal]

I see one problematic (even though unlikely) case with resubmitting the entire dictionary from past stages on every next stage: it's when two subsequent stages happen to use the same parameter name, each for its own purposes; in particular, you cannot stack two identical (say, m.login.password) auth stages one after another ("2-password authentication" - I actually had such 2FA in one of my non-Matrix projects).

Having to resubmit the whole (accumulating) dictionary every time (besides being a waste of bandwidth and, as mentioned above, an additional burden on the client which we usually try to avoid in Matrix) means that stages within a flow should be coordinated tighter, comparing to the case when each stage only contains the session key and its own data.

(Update: the above is child's blabber and doesn't help the discussion.)

[bmarty]

Maybe the problem here is that m.login.password is not a valid stage for registration. User name and user password is part of the (first) sent body but not in auth data.

[clokep]

I see one problematic (even though unlikely) case with resubmitting the entire dictionary from past stages on every next stage:

It isn't about submitting past stage's data, it is about the data necessary for the endpoint to be processed once auth is complete. So anything that is not in the auth dict. (Which happens to include password for registration, which makes this conversation confusing.)

[bmarty]

FTR Dendrite current implementation has the same problem than Conduit: we cannot create account with Element Android

[aaronraimist]

It would be great to fix this soon or even just temporarily edit the error message to say something like "registration only works with Synapse servers right now".

Today someone was trying to setup a Dendrite server and spent hours trying to reconfigure it because they thought their server was broken but it was just this bug.

[timokoesters]

I made Conduit's UIAA implementation work like synapse's:
The first request to a uiaa endpoint is saved and associated with a uiaa session.
If another request to the uiaa endpoint is made with that uiaa session, we add all top level fields from the first request to the request if the field didn't exist already.

I think we can close this issue when an MSC for this behavior is accepted