Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework how email verification works #3784

Merged
merged 28 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
f84ccda
Fix the service worker not loading in dev mode
sandhose Jan 9, 2025
5d69b34
frontend: simplify email list
sandhose Jan 9, 2025
8db3642
Add more stories for the account index page
sandhose Jan 9, 2025
ee33e9c
Remove the primary email address concept
sandhose Jan 9, 2025
75526ff
storage: new email authentication codes
sandhose Jan 9, 2025
1f83b39
Remove the dedicated page to add an email address
sandhose Jan 9, 2025
0513f19
Rip out the email verification codes
sandhose Jan 9, 2025
5f5fc44
Job to send the new email authentication codes
sandhose Jan 10, 2025
23b019c
GraphQL API to use the new email authentication codes
sandhose Jan 10, 2025
e0f4882
Use the new GraphQL APIs in the frontend to add emails
sandhose Jan 10, 2025
dbb5316
Data model and storage layer for storing user registrations
sandhose Jan 13, 2025
3da27af
Move the registration-related views into a sub-module
sandhose Jan 14, 2025
a294b37
Fix the post auth action being lost during the registration flow
sandhose Jan 14, 2025
0bedaf3
Make the password registration create a user_registration
sandhose Jan 14, 2025
f8517a5
Implement email verification in the registration flow
sandhose Jan 14, 2025
621b648
Check that the email isn't used during the registration process
sandhose Jan 14, 2025
36aa1a0
Move the finishing of registration to a dedicated view
sandhose Jan 15, 2025
f50a386
Registration step to set a display name
sandhose Jan 15, 2025
5851584
Link the registration to the browser through a signed cookie
sandhose Jan 15, 2025
02db622
Expire registration sessions after an hour
sandhose Jan 15, 2025
1ec6192
Check with the homeserver the username is still available before regi…
sandhose Jan 15, 2025
ef74d47
Cleanup the unverified emails from the database
sandhose Jan 15, 2025
6092efe
Merge branch 'main' into quenting/optional-email
sandhose Jan 20, 2025
ef077d0
Rate-limit email authentications
sandhose Jan 23, 2025
7e6ab8f
Disclose that email is already in use after verification
sandhose Jan 23, 2025
244ec18
Merge remote-tracking branch 'origin/main' into quenting/optional-email
sandhose Jan 23, 2025
a83cdfb
Clarify that VerifyEmailJob is kept for flushing old jobs
sandhose Jan 23, 2025
8d50088
Apply code style suggestion
sandhose Jan 23, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion biome.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"frontend/src/gql/**",
"frontend/src/routeTree.gen.ts",
"frontend/.storybook/locales.ts",
"frontend/.storybook/mockServiceWorker.js",
"frontend/.storybook/public/mockServiceWorker.js",
"frontend/locales/*.json",
"**/coverage/**",
"**/dist/**"
Expand Down
7 changes: 7 additions & 0 deletions crates/axum-utils/src/cookies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,13 @@ impl CookieJar {
self
}

/// Remove a cookie from the jar
#[must_use]
pub fn remove(mut self, key: &str) -> Self {
self.inner = self.inner.remove(key.to_owned());
self
}

/// Load and deserialize a cookie from the jar
///
/// Returns `None` if the cookie is not present
Expand Down
50 changes: 4 additions & 46 deletions crates/cli/src/commands/manage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ enum Subcommand {
/// Add an email address to the specified user
AddEmail { username: String, email: String },

/// Mark email address as verified
/// [DEPRECATED] Mark email address as verified
VerifyEmail { username: String, email: String },

/// Set a user password
Expand Down Expand Up @@ -252,15 +252,8 @@ impl Options {
.await?
};

let email = repo.user_email().mark_as_verified(&clock, email).await?;

// If the user has no primary email, set this one as primary.
if user.primary_user_email_id.is_none() {
repo.user_email().set_as_primary(&email).await?;
}

repo.into_inner().commit().await?;
info!(?email, "Email added and marked as verified");
info!(?email, "Email added");

Ok(ExitCode::SUCCESS)
}
Expand All @@ -273,31 +266,7 @@ impl Options {
)
.entered();

let database_config = DatabaseConfig::extract_or_default(figment)?;
let mut conn = database_connection_from_config(&database_config).await?;
let txn = conn.begin().await?;
let mut repo = PgRepository::from_conn(txn);

let user = repo
.user()
.find_by_username(&username)
.await?
.context("User not found")?;

let email = repo
.user_email()
.find(&user, &email)
.await?
.context("Email not found")?;
let email = repo.user_email().mark_as_verified(&clock, email).await?;

// If the user has no primary email, set this one as primary.
if user.primary_user_email_id.is_none() {
repo.user_email().set_as_primary(&email).await?;
}

repo.into_inner().commit().await?;
info!(?email, "Email marked as verified");
tracing::warn!("The 'verify-email' command is deprecated and will be removed in a future version. Use 'add-email' instead.");

Ok(ExitCode::SUCCESS)
}
Expand Down Expand Up @@ -943,20 +912,9 @@ impl UserCreationRequest<'_> {
}

for email in emails {
let user_email = repo
.user_email()
repo.user_email()
.add(rng, clock, &user, email.to_string())
.await?;

let user_email = repo
.user_email()
.mark_as_verified(clock, user_email)
.await?;

if user.primary_user_email_id.is_none() {
repo.user_email().set_as_primary(&user_email).await?;
user.primary_user_email_id = Some(user_email.id);
}
}

for (provider, subject) in upstream_provider_mappings {
Expand Down
11 changes: 0 additions & 11 deletions crates/cli/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,6 @@ fn map_claims_imports(
action: map_import_action(config.email.action),
template: config.email.template.clone(),
},
verify_email: match config.email.set_email_verification {
mas_config::UpstreamOAuth2SetEmailVerification::Always => {
mas_data_model::UpsreamOAuthProviderSetEmailVerification::Always
}
mas_config::UpstreamOAuth2SetEmailVerification::Never => {
mas_data_model::UpsreamOAuthProviderSetEmailVerification::Never
}
mas_config::UpstreamOAuth2SetEmailVerification::Import => {
mas_data_model::UpsreamOAuthProviderSetEmailVerification::Import
}
},
account_name: mas_data_model::UpstreamOAuthProviderSubjectPreference {
template: config.account_name.template.clone(),
},
Expand Down
1 change: 0 additions & 1 deletion crates/config/src/sections/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ pub use self::{
EmailImportPreference as UpstreamOAuth2EmailImportPreference,
ImportAction as UpstreamOAuth2ImportAction, PkceMethod as UpstreamOAuth2PkceMethod,
ResponseMode as UpstreamOAuth2ResponseMode,
SetEmailVerification as UpstreamOAuth2SetEmailVerification,
TokenAuthMethod as UpstreamOAuth2TokenAuthMethod, UpstreamOAuth2Config,
},
};
Expand Down
79 changes: 78 additions & 1 deletion crates/config/src/sections/rate_limiting.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 New Vector Ltd.
// Copyright 2024, 2025 New Vector Ltd.
// Copyright 2024 The Matrix.org Foundation C.I.C.
//
// SPDX-License-Identifier: AGPL-3.0-only
Expand All @@ -18,13 +18,19 @@ pub struct RateLimitingConfig {
/// Account Recovery-specific rate limits
#[serde(default)]
pub account_recovery: AccountRecoveryRateLimitingConfig,

/// Login-specific rate limits
#[serde(default)]
pub login: LoginRateLimitingConfig,

/// Controls how many registrations attempts are permitted
/// based on source address.
#[serde(default = "default_registration")]
pub registration: RateLimiterConfiguration,

/// Email authentication-specific rate limits
#[serde(default)]
pub email_authentication: EmailauthenticationRateLimitingConfig,
}

#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
Expand All @@ -37,6 +43,7 @@ pub struct LoginRateLimitingConfig {
/// change their own password.
#[serde(default = "default_login_per_ip")]
pub per_ip: RateLimiterConfiguration,

/// Controls how many login attempts are permitted
/// based on the account that is being attempted to be logged into.
/// This can protect against a distributed brute force attack
Expand All @@ -58,6 +65,7 @@ pub struct AccountRecoveryRateLimitingConfig {
/// Note: this limit also applies to re-sends.
#[serde(default = "default_account_recovery_per_ip")]
pub per_ip: RateLimiterConfiguration,

/// Controls how many account recovery attempts are permitted
/// based on the e-mail address entered into the recovery form.
/// This can protect against causing e-mail spam to one target.
Expand All @@ -67,6 +75,35 @@ pub struct AccountRecoveryRateLimitingConfig {
pub per_address: RateLimiterConfiguration,
}

#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
pub struct EmailauthenticationRateLimitingConfig {
/// Controls how many email authentication attempts are permitted
/// based on the source IP address.
/// This can protect against causing e-mail spam to many targets.
#[serde(default = "default_email_authentication_per_ip")]
pub per_ip: RateLimiterConfiguration,

/// Controls how many email authentication attempts are permitted
/// based on the e-mail address entered into the authentication form.
/// This can protect against causing e-mail spam to one target.
///
/// Note: this limit also applies to re-sends.
#[serde(default = "default_email_authentication_per_address")]
pub per_address: RateLimiterConfiguration,

/// Controls how many authentication emails are permitted to be sent per
/// authentication session. This ensures not too many authentication codes
/// are created for the same authentication session.
#[serde(default = "default_email_authentication_emails_per_session")]
pub emails_per_session: RateLimiterConfiguration,

/// Controls how many code authentication attempts are permitted per
/// authentication session. This can protect against brute-forcing the
/// code.
#[serde(default = "default_email_authentication_attempt_per_session")]
pub attempt_per_session: RateLimiterConfiguration,
}

#[derive(Copy, Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
pub struct RateLimiterConfiguration {
/// A one-off burst of actions that the user can perform
Expand Down Expand Up @@ -193,12 +230,41 @@ fn default_account_recovery_per_address() -> RateLimiterConfiguration {
}
}

fn default_email_authentication_per_ip() -> RateLimiterConfiguration {
RateLimiterConfiguration {
burst: NonZeroU32::new(5).unwrap(),
per_second: 1.0 / 60.0,
}
}

fn default_email_authentication_per_address() -> RateLimiterConfiguration {
RateLimiterConfiguration {
burst: NonZeroU32::new(3).unwrap(),
per_second: 1.0 / 3600.0,
}
}

fn default_email_authentication_emails_per_session() -> RateLimiterConfiguration {
RateLimiterConfiguration {
burst: NonZeroU32::new(2).unwrap(),
per_second: 1.0 / 300.0,
}
}

fn default_email_authentication_attempt_per_session() -> RateLimiterConfiguration {
RateLimiterConfiguration {
burst: NonZeroU32::new(10).unwrap(),
per_second: 1.0 / 60.0,
}
}

impl Default for RateLimitingConfig {
fn default() -> Self {
RateLimitingConfig {
login: LoginRateLimitingConfig::default(),
registration: default_registration(),
account_recovery: AccountRecoveryRateLimitingConfig::default(),
email_authentication: EmailauthenticationRateLimitingConfig::default(),
}
}
}
Expand All @@ -220,3 +286,14 @@ impl Default for AccountRecoveryRateLimitingConfig {
}
}
}

impl Default for EmailauthenticationRateLimitingConfig {
fn default() -> Self {
EmailauthenticationRateLimitingConfig {
per_ip: default_email_authentication_per_ip(),
per_address: default_email_authentication_per_address(),
emails_per_session: default_email_authentication_emails_per_session(),
attempt_per_session: default_email_authentication_attempt_per_session(),
}
}
}
31 changes: 1 addition & 30 deletions crates/config/src/sections/upstream_oauth2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,29 +180,6 @@ impl ImportAction {
}
}

/// Should the email address be marked as verified
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default, JsonSchema)]
#[serde(rename_all = "lowercase")]
pub enum SetEmailVerification {
/// Mark the email address as verified
Always,

/// Don't mark the email address as verified
Never,

/// Mark the email address as verified if the upstream provider says it is
/// through the `email_verified` claim
#[default]
Import,
}

impl SetEmailVerification {
#[allow(clippy::trivially_copy_pass_by_ref)]
const fn is_default(&self) -> bool {
matches!(self, SetEmailVerification::Import)
}
}

/// What should be done for the subject attribute
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default, JsonSchema)]
pub struct SubjectImportPreference {
Expand Down Expand Up @@ -271,17 +248,11 @@ pub struct EmailImportPreference {
/// If not provided, the default template is `{{ user.email }}`
#[serde(default, skip_serializing_if = "Option::is_none")]
pub template: Option<String>,

/// Should the email address be marked as verified
#[serde(default, skip_serializing_if = "SetEmailVerification::is_default")]
pub set_email_verification: SetEmailVerification,
}

impl EmailImportPreference {
const fn is_default(&self) -> bool {
self.action.is_default()
&& self.template.is_none()
&& self.set_email_verification.is_default()
self.action.is_default() && self.template.is_none()
}
}

Expand Down
15 changes: 8 additions & 7 deletions crates/data-model/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,17 @@ pub use self::{
AccessToken, AccessTokenState, RefreshToken, RefreshTokenState, TokenFormatError, TokenType,
},
upstream_oauth2::{
UpsreamOAuthProviderSetEmailVerification, UpstreamOAuthAuthorizationSession,
UpstreamOAuthAuthorizationSessionState, UpstreamOAuthLink, UpstreamOAuthProvider,
UpstreamOAuthProviderClaimsImports, UpstreamOAuthProviderDiscoveryMode,
UpstreamOAuthProviderImportAction, UpstreamOAuthProviderImportPreference,
UpstreamOAuthProviderPkceMode, UpstreamOAuthProviderResponseMode,
UpstreamOAuthProviderSubjectPreference, UpstreamOAuthProviderTokenAuthMethod,
UpstreamOAuthAuthorizationSession, UpstreamOAuthAuthorizationSessionState,
UpstreamOAuthLink, UpstreamOAuthProvider, UpstreamOAuthProviderClaimsImports,
UpstreamOAuthProviderDiscoveryMode, UpstreamOAuthProviderImportAction,
UpstreamOAuthProviderImportPreference, UpstreamOAuthProviderPkceMode,
UpstreamOAuthProviderResponseMode, UpstreamOAuthProviderSubjectPreference,
UpstreamOAuthProviderTokenAuthMethod,
},
user_agent::{DeviceType, UserAgent},
users::{
Authentication, AuthenticationMethod, BrowserSession, Password, User, UserEmail,
UserEmailVerification, UserEmailVerificationState, UserRecoverySession, UserRecoveryTicket,
UserEmailAuthentication, UserEmailAuthenticationCode, UserRecoverySession,
UserRecoveryTicket, UserRegistration, UserRegistrationPassword,
},
};
1 change: 0 additions & 1 deletion crates/data-model/src/upstream_oauth2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ pub use self::{
ImportPreference as UpstreamOAuthProviderImportPreference,
PkceMode as UpstreamOAuthProviderPkceMode,
ResponseMode as UpstreamOAuthProviderResponseMode,
SetEmailVerification as UpsreamOAuthProviderSetEmailVerification,
SubjectPreference as UpstreamOAuthProviderSubjectPreference,
TokenAuthMethod as UpstreamOAuthProviderTokenAuthMethod, UpstreamOAuthProvider,
},
Expand Down
29 changes: 0 additions & 29 deletions crates/data-model/src/upstream_oauth2/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,32 +263,6 @@ impl UpstreamOAuthProvider {
}
}

/// Whether to set the email as verified when importing it from the upstream
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
#[serde(rename_all = "lowercase")]
pub enum SetEmailVerification {
/// Set the email as verified
Always,

/// Never set the email as verified
Never,

/// Set the email as verified if the upstream provider claims it is verified
#[default]
Import,
}

impl SetEmailVerification {
#[must_use]
pub fn should_mark_as_verified(&self, upstream_verified: bool) -> bool {
match self {
Self::Always => true,
Self::Never => false,
Self::Import => upstream_verified,
}
}
}

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
pub struct ClaimsImports {
#[serde(default)]
Expand All @@ -305,9 +279,6 @@ pub struct ClaimsImports {

#[serde(default)]
pub account_name: SubjectPreference,

#[serde(default)]
pub verify_email: SetEmailVerification,
}

// XXX: this should have another name
Expand Down
Loading
Loading