Spec-compliant localparts for virtual matrix users #1781
Conversation
|
Hey there, I'd absolutely love to have this change included. It has been a long time coming. Obviously we'd need to be careful to support "legacy" identifiers as the old behaviour is widespread but I'd love to see this happen for new deployments. |
| # Typescript build | ||
| FROM node:18 as builder | ||
|
|
||
| RUN apt-get update && apt-get install -y node-gyp --no-install-recommends |
There was a problem hiding this comment.
Do you know what caused this to flag up? It's weird that you found this without changing the deps around at all.
There was a problem hiding this comment.
No idea, it just failed to compile. I wrote this on a brand new VPS. The base images were all fresh downloads. Maybe the upstream image somehow removed node-gyp?
There was a problem hiding this comment.
It seems node:18.17 works node:18.18 is missing node-gyp for some reason.
There was a problem hiding this comment.
We've had this before where minor image versions break (and then because we target a major version, you end up not noticing).
The change looks good then. I think in the future we're just going to have to start pinning versions strictly as we can't be having this.
src/irc/IrcServer.ts
Outdated
| return match[1]; | ||
|
|
||
| // https://spec.matrix.org/v1.8/appendices/#mapping-from-other-character-sets | ||
| const unescaped = match[1].replaceAll(/=([0-9a-f][0-9a-f])/g, |
There was a problem hiding this comment.
I'd suggest making this behaviour configurable for now so old deployments don't break (and defaulting to off). In a future PR we can start defaulting new deployments to using it, and somewhere down the line we should stop supporting the old mechansim.
Make sense?
There was a problem hiding this comment.
Yes, makes perfect sense.
I haven't looked at any configuration code yet. I will update this with a proposal for a configuration option
src/irc/IrcServer.ts
Outdated
| federate: boolean; | ||
| }; | ||
| matrixClients: { | ||
| matrixIdNormalization: number, |
There was a problem hiding this comment.
I thought that I should make this an integer, in case there are more schemes in the future
Signed-off-by: Kai Bleeke <bleeke.kai@gmail.com>
Signed-off-by: Kai Bleeke <bleeke.kai@gmail.com>
Signed-off-by: Kai Bleeke <bleeke.kai@gmail.com>
| localPartCharacterMapping: | ||
| type: "integer" | ||
| minimum: 0 | ||
| maximum: 1 |
There was a problem hiding this comment.
Is there a way to set a default value here so it doesn't break existing configs? Or does that happens automatically?
2 participants
Closes #1780
I would like to have the option to create localparts for virtual matrix users that only contain valid characters according to
https://spec.matrix.org/v1.8/appendices/#user-identifiers
The spec then suggest to map characters like
https://spec.matrix.org/v1.8/appendices/#mapping-from-other-character-sets
Well, the "have the option" part is still missing from the config file.
If you are generally interested in accepting a PR for this, I will do my best to comply with Contributing.md