feat: Add support for email address management #1565
Labels
No labels
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1565
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ginger/email-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request adds support for:
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
@ -0,0 +1,5 @@{% extends "_base.txt" %}Some inconsistency with the use of .j2 jinja extension between here and the web crate
The j2 extension makes askama think the templates are HTML and escape them accordingly, which leads to ampersands in URL queries being replaced with
&.just a few preliminary things, didn't get chance to review the UIAA service changes
@ -0,0 +51,4 @@InsecureClientIp(client): InsecureClientIp,body: Ruma<get_username_availability::v3::Request>,) -> Result<get_username_availability::v3::Response> {// workaround for https://github.com/matrix-org/matrix-appservice-irc/issues/1780 due to inactivity of fixing the issueIf we're refactoring we might as well just remove this workaround and the associated code below it. Just seems like a liability for no good reason.
AAAA bad, someone will be using this. I know MetaBrainz have appservice-irc deployed
https://xkcd.com/1172/
i dont care 👍 upstreams should fix their bugs, we shouldn't be fixing them for them, especially when it's in a security hot zone like user registration
I guess people should stop using abandoned software. Hard ask tho 😔
getting people to not use abandoned software is literally what created this project 🧌
@ -0,0 +185,4 @@services.threepid.associate_localpart_email(user_id.localpart(), &email).await?;This probably needs an explicit rollback if the
services.users.createfunction fails below@ -0,0 +499,4 @@}// Workaround for https://github.com/matrix-org/matrix-appservice-irc/issues/1780 due to inactivity of fixing the issuelet is_matrix_appservice_irc = appservice_info.is_some_and(|appservice| {should also remove this (see earlier comment)
@ -0,0 +550,4 @@Ok(user_id)} else {// The user is a guest or is lacking in creativity. Generate a username forwhat does "creativity" mean??
@ -0,0 +555,4 @@loop {let user_id = UserId::parse_with_server_name(utils::random_string(RANDOM_USER_ID_LENGTH).to_lowercase(),I'm gonna need a justification for potentially burdening someone with an immutable random string for their user ID. I hate that my spotify user ID is 32 random chars and can't be changed so im sure someone ending up here would hate this too
From https://spec.matrix.org/v1.17/client-server-api/#post_matrixclientv3register regarding the
usernameparameter:I suspect the meaning of this was to generate the localpart based on the email provided, not random. iirc Synapse does something like this (which is why you see user IDs like
@johnathan=46sunderland98=40gmail=46com:matrix.example), in theory we could just pull the email's identifier anyway.I'm not sure there's another situation where
localpartis ever empty. I guess if no identifiable 3pid is included and the user tries to register an empty username then they can be saddled with a random string for doing something dumb@ -415,13 +415,6 @@ impl<'a, 'de: 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {tracing::instrument(level = "trace", skip_all, fields(?self.buf)))]fn deserialize_any<V: Visitor<'de>>(self, visitor: V) -> Result<V::Value> {debug_assert_eq!(?
This was causing panics when trying to register an account in a debug build because one type name had a lifetime parameter and the other didn't. I'm not sure what it was testing, but if it was panicking in code I didn't even touch that was working fine beforehand, I doubt that it was doing its job correctly.
Do we have any wal of removing expored tokens eventually? Probably an important TODO to write down at least
oh kay then, in memory is one solution to this
@ -0,0 +64,4 @@send_attempt: usize,) -> Result<OwnedSessionId> {let mailer = self.services.mailer.expect_mailer()?;let mut sessions = self.sessions.lock().await;This will hold the lock over the whole scope, including while sending the email
@ -0,0 +141,4 @@session_id: &SessionId,supplied_token: &str,) -> Result<(), Cow<'static, str>> {let mut sessions = self.sessions.lock().await;ditto, although less of an issue here
The point of the function is to possibly mutate the session in question, and we can't do that without holding the lock.
The only thing I really have concerns with is the in-memory session store, but that's fine for now. Just fix the lock held across the mailer I think?
Fixed.
9d651bed923564c4ea4a3564c4ea4a854901d79awoof
ginger wants me to break stuff before approving