matrix-org / prosody-mod-auth-matrix-user-verification

Matrix user verification auth for Prosody
Apache License 2.0
24 stars 12 forks source link

Should the server_name really be optional? #12

Open anoadragon453 opened 3 years ago

anoadragon453 commented 3 years ago

In the README under Usage, we state that the server_name parameter is optional. If it is, we assume that the Matrix User Verification Service has been configured to only speak to one server.

It's up to the client to put this parameter in the token or not, however it's not clear how the client would even know this information. Thus they'll end up sending the server_name parameter every time.

Additionally, it appears that when attempting to leave out the server_name parameter, the UVS doesn't even use the configured server anyhow:

{
  level: 'warn',
  message: 'Refusing to check token which is not for the server we have configured: undefined',
  timestamp: '2021-05-20T16:25:08.702Z'
}

I propose that we just make server_name a required parameter in the JWT, unless there's a reason to keep it as optional.

jaywink commented 3 years ago

Yes I agree, this change makes sense. Element clients (afaict the only ones implementing Jitsi auth using this module) already send the server_name parameter afaik, but we should double check).

Additionally, it appears that when attempting to leave out the server_name parameter, the UVS doesn't even use the configured server anyhow:

Looking at the code it seems like there is a bug indeed, the request validation correclty ignores matrix_server_name missing if one is configured via env, but the verify logic doesn't match this completely. The plan was to make matrix_server_name required in the next release anyway, so will fix that.