Skip to content
Permalink

Comparing changes

This is a direct comparison between two commits made in this repository or its related repositories. View the default comparison for this range or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: element-hq/matrix-authentication-service
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 5638b673347fc95d2fc853023ff37d3261090306
Choose a base ref
..
head repository: element-hq/matrix-authentication-service
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: d65286dedd43c22d6d6b1c30e128732422228e47
Choose a head ref
2 changes: 1 addition & 1 deletion crates/data-model/src/users.rs
Original file line number Diff line number Diff line change
@@ -204,7 +204,7 @@ pub struct UserRegistrationPassword {
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
pub struct UserRegistration {
pub id: Ulid,
pub username: Option<String>,
pub username: String,
pub display_name: Option<String>,
pub terms_url: Option<Url>,
pub email_authentication_id: Option<Ulid>,
16 changes: 9 additions & 7 deletions crates/handlers/src/views/register/password.rs
Original file line number Diff line number Diff line change
@@ -304,12 +304,14 @@ pub(crate) async fn post(
.transpose()?;
let registration = repo
.user_registration()
.add(&mut rng, &clock, ip_address, user_agent, post_auth_action)
.await?;

let registration = repo
.user_registration()
.set_username(registration, form.username)
.add(
&mut rng,
&clock,
form.username,
ip_address,
user_agent,
post_auth_action,
)
.await?;

let registration = if let Some(tos_uri) = &site_config.tos_uri {
@@ -483,7 +485,7 @@ mod tests {
// There should be a new registration in the database
let mut repo = state.repository().await.unwrap();
let registration = repo.user_registration().lookup(id).await.unwrap().unwrap();
assert_eq!(registration.username, Some("john".to_owned()));
assert_eq!(registration.username, "john".to_owned());
assert!(registration.password.is_some());

let email_authentication = repo
8 changes: 4 additions & 4 deletions crates/handlers/src/views/register/steps/verify_email.rs
Original file line number Diff line number Diff line change
@@ -191,10 +191,10 @@ pub(crate) async fn post(
.await?;

// XXX: this should move somewhere else, and it doesn't check for uniqueness
let Some(username) = registration.username else {
todo!()
};
let user = repo.user().add(&mut rng, &clock, username).await?;
let user = repo
.user()
.add(&mut rng, &clock, registration.username)
.await?;
let user_session = repo
.browser_session()
.add(&mut rng, &clock, &user, user_agent)

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@ CREATE TABLE "user_registrations" (
"post_auth_action" JSONB,

-- The username the user asked for
"username" TEXT,
"username" TEXT NOT NULL,

-- The display name the user asked for
"display_name" TEXT,
121 changes: 13 additions & 108 deletions crates/storage-pg/src/user/registration.rs
Original file line number Diff line number Diff line change
@@ -38,7 +38,7 @@ struct UserRegistrationLookup {
ip_address: Option<IpAddr>,
user_agent: Option<String>,
post_auth_action: Option<serde_json::Value>,
username: Option<String>,
username: String,
display_name: Option<String>,
terms_url: Option<String>,
email_authentication_id: Option<Uuid>,
@@ -160,6 +160,7 @@ impl UserRegistrationRepository for PgUserRegistrationRepository<'_> {
&mut self,
rng: &mut (dyn RngCore + Send),
clock: &dyn Clock,
username: String,
ip_address: Option<IpAddr>,
user_agent: Option<UserAgent>,
post_auth_action: Option<serde_json::Value>,
@@ -175,14 +176,16 @@ impl UserRegistrationRepository for PgUserRegistrationRepository<'_> {
, ip_address
, user_agent
, post_auth_action
, username
, created_at
)
VALUES ($1, $2, $3, $4, $5)
VALUES ($1, $2, $3, $4, $5, $6)
"#,
Uuid::from(id),
ip_address as Option<IpAddr>,
user_agent.as_deref(),
post_auth_action,
username,
created_at,
)
.traced()
@@ -196,49 +199,14 @@ impl UserRegistrationRepository for PgUserRegistrationRepository<'_> {
post_auth_action,
created_at,
completed_at: None,
username: None,
username,
display_name: None,
terms_url: None,
email_authentication_id: None,
password: None,
})
}

#[tracing::instrument(
name = "db.user_registration.set_username",
skip_all,
fields(
db.query.text,
user_registration.id = %user_registration.id,
user_registration.username = username,
),
err,
)]
async fn set_username(
&mut self,
mut user_registration: UserRegistration,
username: String,
) -> Result<UserRegistration, Self::Error> {
let res = sqlx::query!(
r#"
UPDATE user_registrations
SET username = $2
WHERE user_registration_id = $1 AND completed_at IS NULL
"#,
Uuid::from(user_registration.id),
username,
)
.traced()
.execute(&mut *self.conn)
.await?;

DatabaseError::ensure_affected_rows(&res, 1)?;

user_registration.username = Some(username);

Ok(user_registration)
}

#[tracing::instrument(
name = "db.user_registration.set_display_name",
skip_all,
@@ -443,13 +411,13 @@ mod tests {

let registration = repo
.user_registration()
.add(&mut rng, &clock, None, None, None)
.add(&mut rng, &clock, "alice".to_owned(), None, None, None)
.await
.unwrap();

assert_eq!(registration.created_at, clock.now());
assert_eq!(registration.completed_at, None);
assert_eq!(registration.username, None);
assert_eq!(registration.username, "alice");
assert_eq!(registration.display_name, None);
assert_eq!(registration.terms_url, None);
assert_eq!(registration.email_authentication_id, None);
@@ -517,6 +485,7 @@ mod tests {
.add(
&mut rng,
&clock,
"alice".to_owned(),
Some(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1))),
Some(UserAgent::parse("Mozilla/5.0".to_owned())),
Some(serde_json::json!({"action": "continue_compat_sso_login", "id": "01FSHN9AG0MKGTBNZ16RDR3PVY"})),
@@ -551,70 +520,6 @@ mod tests {
assert_eq!(lookup.post_auth_action, registration.post_auth_action);
}

#[sqlx::test(migrator = "crate::MIGRATOR")]
async fn test_set_username(pool: PgPool) {
let mut rng = ChaChaRng::seed_from_u64(42);
let clock = MockClock::default();

let mut repo = PgRepository::from_pool(&pool).await.unwrap().boxed();

let registration = repo
.user_registration()
.add(&mut rng, &clock, None, None, None)
.await
.unwrap();

assert_eq!(registration.username, None);

let registration = repo
.user_registration()
.set_username(registration, "alice".to_owned())
.await
.unwrap();

assert_eq!(registration.username, Some("alice".to_owned()));

let lookup = repo
.user_registration()
.lookup(registration.id)
.await
.unwrap()
.unwrap();

assert_eq!(lookup.username, registration.username);

// Setting it again should work
let registration = repo
.user_registration()
.set_username(registration, "bob".to_owned())
.await
.unwrap();

assert_eq!(registration.username, Some("bob".to_owned()));

let lookup = repo
.user_registration()
.lookup(registration.id)
.await
.unwrap()
.unwrap();

assert_eq!(lookup.username, registration.username);

// Can't set it once completed
let registration = repo
.user_registration()
.complete(&clock, registration)
.await
.unwrap();

let res = repo
.user_registration()
.set_username(registration, "charlie".to_owned())
.await;
assert!(res.is_err());
}

#[sqlx::test(migrator = "crate::MIGRATOR")]
async fn test_set_display_name(pool: PgPool) {
let mut rng = ChaChaRng::seed_from_u64(42);
@@ -624,7 +529,7 @@ mod tests {

let registration = repo
.user_registration()
.add(&mut rng, &clock, None, None, None)
.add(&mut rng, &clock, "alice".to_owned(), None, None, None)
.await
.unwrap();

@@ -688,7 +593,7 @@ mod tests {

let registration = repo
.user_registration()
.add(&mut rng, &clock, None, None, None)
.add(&mut rng, &clock, "alice".to_owned(), None, None, None)
.await
.unwrap();

@@ -758,7 +663,7 @@ mod tests {

let registration = repo
.user_registration()
.add(&mut rng, &clock, None, None, None)
.add(&mut rng, &clock, "alice".to_owned(), None, None, None)
.await
.unwrap();

@@ -845,7 +750,7 @@ mod tests {

let registration = repo
.user_registration()
.add(&mut rng, &clock, None, None, None)
.add(&mut rng, &clock, "alice".to_owned(), None, None, None)
.await
.unwrap();

27 changes: 3 additions & 24 deletions crates/storage/src/user/registration.rs
Original file line number Diff line number Diff line change
@@ -41,6 +41,7 @@ pub trait UserRegistrationRepository: Send + Sync {
///
/// * `rng`: The random number generator to use
/// * `clock`: The clock used to generate timestamps
/// * `username`: The username of the user
/// * `ip_address`: The IP address of the user agent, if any
/// * `user_agent`: The user agent of the user agent, if any
/// * `post_auth_action`: The post auth action to execute after the
@@ -53,30 +54,12 @@ pub trait UserRegistrationRepository: Send + Sync {
&mut self,
rng: &mut (dyn RngCore + Send),
clock: &dyn Clock,
username: String,
ip_address: Option<IpAddr>,
user_agent: Option<UserAgent>,
post_auth_action: Option<serde_json::Value>,
) -> Result<UserRegistration, Self::Error>;

/// Set the username of a [`UserRegistration`]
///
/// Returns the updated [`UserRegistration`]
///
/// # Parameters
///
/// * `user_registration`: The [`UserRegistration`] to update
/// * `username`: The username to set
///
/// # Errors
///
/// Returns [`Self::Error`] if the underlying repository fails or if the
/// registration is already completed
async fn set_username(
&mut self,
user_registration: UserRegistration,
username: String,
) -> Result<UserRegistration, Self::Error>;

/// Set the display name of a [`UserRegistration`]
///
/// Returns the updated [`UserRegistration`]
@@ -181,15 +164,11 @@ repository_impl!(UserRegistrationRepository:
&mut self,
rng: &mut (dyn RngCore + Send),
clock: &dyn Clock,
username: String,
ip_address: Option<IpAddr>,
user_agent: Option<UserAgent>,
post_auth_action: Option<serde_json::Value>,
) -> Result<UserRegistration, Self::Error>;
async fn set_username(
&mut self,
user_registration: UserRegistration,
username: String,
) -> Result<UserRegistration, Self::Error>;
async fn set_display_name(
&mut self,
user_registration: UserRegistration,
2 changes: 1 addition & 1 deletion crates/tasks/src/email.rs
Original file line number Diff line number Diff line change
@@ -115,7 +115,7 @@ impl RunnableJob for SendEmailAuthenticationCodeJob {
.parse()
.map_err(JobError::fail)?;
let username_from_session = browser_session.as_ref().map(|s| s.user.username.clone());
let username_from_registration = registration.as_ref().and_then(|r| r.username.clone());
let username_from_registration = registration.as_ref().map(|r| r.username.clone());
let username = username_from_registration.or(username_from_session);
let mailbox = Mailbox::new(username, address);