diff --git a/crates/handlers/src/upstream_oauth2/link.rs b/crates/handlers/src/upstream_oauth2/link.rs index e48ff190b..b37c4fce7 100644 --- a/crates/handlers/src/upstream_oauth2/link.rs +++ b/crates/handlers/src/upstream_oauth2/link.rs @@ -759,10 +759,12 @@ pub(crate) async fn post( Some("username") => form_state.with_error_on_field( mas_templates::UpstreamRegisterFormField::Username, FieldError::Policy { + code: violation.code.map(|c| c.as_str()), message: violation.msg, }, ), _ => form_state.with_error_on_form(FormError::Policy { + code: violation.code.map(|c| c.as_str()), message: violation.msg, }), } diff --git a/crates/handlers/src/views/register.rs b/crates/handlers/src/views/register.rs index 981ff81b2..af7855dca 100644 --- a/crates/handlers/src/views/register.rs +++ b/crates/handlers/src/views/register.rs @@ -154,6 +154,7 @@ pub(crate) async fn post( state.add_error_on_form(FormError::Captcha); } + let mut homeserver_denied_username = false; if form.username.is_empty() { state.add_error_on_field(RegisterFormField::Username, FieldError::Required); } else if repo.user().exists(&form.username).await? { @@ -161,13 +162,14 @@ pub(crate) async fn post( state.add_error_on_field(RegisterFormField::Username, FieldError::Exists); } else if !homeserver.is_localpart_available(&form.username).await? { // The user already exists on the homeserver - // XXX: we may want to return different errors like "this username is reserved" tracing::warn!( username = &form.username, - "User tried to register with a reserved username" + "Homeserver denied username provided by user" ); - state.add_error_on_field(RegisterFormField::Username, FieldError::Exists); + // We defer adding the error on the field, until we know whether we had another + // error from the policy, to avoid showing both + homeserver_denied_username = true; } if form.email.is_empty() { @@ -197,6 +199,7 @@ pub(crate) async fn post( state.add_error_on_field( RegisterFormField::Password, FieldError::Policy { + code: None, message: "Password is too weak".to_owned(), }, ); @@ -216,27 +219,41 @@ pub(crate) async fn post( Some("email") => state.add_error_on_field( RegisterFormField::Email, FieldError::Policy { + code: violation.code.map(|c| c.as_str()), message: violation.msg, }, ), - Some("username") => state.add_error_on_field( - RegisterFormField::Username, - FieldError::Policy { - message: violation.msg, - }, - ), + Some("username") => { + // If the homeserver denied the username, but we also had an error on the policy + // side, we don't want to show both, so we reset the state here + homeserver_denied_username = false; + state.add_error_on_field( + RegisterFormField::Username, + FieldError::Policy { + code: violation.code.map(|c| c.as_str()), + message: violation.msg, + }, + ); + } Some("password") => state.add_error_on_field( RegisterFormField::Password, FieldError::Policy { + code: violation.code.map(|c| c.as_str()), message: violation.msg, }, ), _ => state.add_error_on_form(FormError::Policy { + code: violation.code.map(|c| c.as_str()), message: violation.msg, }), } } + if homeserver_denied_username { + // XXX: we may want to return different errors like "this username is reserved" + state.add_error_on_field(RegisterFormField::Username, FieldError::Exists); + } + if state.is_valid() { // Check the rate limit if we are about to process the form if let Err(e) = limiter.check_registration(requester) { diff --git a/crates/policy/src/lib.rs b/crates/policy/src/lib.rs index 950957692..9ffe2f511 100644 --- a/crates/policy/src/lib.rs +++ b/crates/policy/src/lib.rs @@ -17,7 +17,7 @@ use thiserror::Error; use tokio::io::{AsyncRead, AsyncReadExt}; use self::model::{AuthorizationGrantInput, ClientRegistrationInput, EmailInput, RegisterInput}; -pub use self::model::{EvaluationResult, Violation}; +pub use self::model::{Code as ViolationCode, EvaluationResult, Violation}; use crate::model::GrantType; #[derive(Debug, Error)] diff --git a/crates/policy/src/model.rs b/crates/policy/src/model.rs index c8d46599d..aebca8928 100644 --- a/crates/policy/src/model.rs +++ b/crates/policy/src/model.rs @@ -13,6 +13,45 @@ use mas_data_model::{Client, User}; use oauth2_types::{registration::VerifiedClientMetadata, scope::Scope}; use serde::{Deserialize, Serialize}; +/// A well-known policy code. +#[derive(Deserialize, Debug, Clone, Copy)] +#[serde(rename_all = "kebab-case")] +#[cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema))] +pub enum Code { + /// The username is too short. + UsernameTooShort, + + /// The username is too long. + UsernameTooLong, + + /// The username contains invalid characters. + UsernameInvalidChars, + + /// The username contains only numeric characters. + UsernameAllNumeric, + + /// The email domain is not allowed. + EmailDomainNotAllowed, + + /// The email domain is banned. + EmailDomainBanned, +} + +impl Code { + /// Returns the code as a string + #[must_use] + pub fn as_str(&self) -> &'static str { + match self { + Self::UsernameTooShort => "username-too-short", + Self::UsernameTooLong => "username-too-long", + Self::UsernameInvalidChars => "username-invalid-chars", + Self::UsernameAllNumeric => "username-all-numeric", + Self::EmailDomainNotAllowed => "email-domain-not-allowed", + Self::EmailDomainBanned => "email-domain-banned", + } + } +} + /// A single violation of a policy. #[derive(Deserialize, Debug)] #[cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema))] @@ -20,6 +59,7 @@ pub struct Violation { pub msg: String, pub redirect_uri: Option, pub field: Option, + pub code: Option, } /// The result of a policy evaluation. diff --git a/crates/templates/src/context.rs b/crates/templates/src/context.rs index 70fc16d0f..eb10e9592 100644 --- a/crates/templates/src/context.rs +++ b/crates/templates/src/context.rs @@ -462,6 +462,7 @@ impl TemplateContext for LoginContext { .with_error_on_field( LoginFormField::Password, FieldError::Policy { + code: None, message: "password too short".to_owned(), }, ), diff --git a/crates/templates/src/forms.rs b/crates/templates/src/forms.rs index a018633ba..2b769e122 100644 --- a/crates/templates/src/forms.rs +++ b/crates/templates/src/forms.rs @@ -36,6 +36,9 @@ pub enum FieldError { /// Denied by the policy Policy { + /// Well-known policy code + code: Option<&'static str>, + /// Message for this policy violation message: String, }, @@ -59,6 +62,9 @@ pub enum FormError { /// Denied by the policy Policy { + /// Well-known policy code + code: Option<&'static str>, + /// Message for this policy violation message: String, }, diff --git a/policies/email/email.rego b/policies/email/email.rego index 00103da26..24b1d94b4 100644 --- a/policies/email/email.rego +++ b/policies/email/email.rego @@ -25,12 +25,12 @@ domain_allowed if { # METADATA # entrypoint: true -violation contains {"msg": "email domain is not allowed"} if { +violation contains {"code": "email-domain-not-allowed", "msg": "email domain is not allowed"} if { not domain_allowed } # Deny emails with their domain in the domains banlist -violation contains {"msg": "email domain is banned"} if { +violation contains {"code": "email-domain-banned", "msg": "email domain is banned"} if { [_, domain] := split(input.email, "@") some banned_domain in data.banned_domains glob.match(banned_domain, ["."], domain) diff --git a/policies/register/register.rego b/policies/register/register.rego index 1fb400aa5..0fb36bf37 100644 --- a/policies/register/register.rego +++ b/policies/register/register.rego @@ -17,16 +17,26 @@ mxid(username, server_name) := sprintf("@%s:%s", [username, server_name]) # METADATA # entrypoint: true -violation contains {"field": "username", "msg": "username too short"} if { +violation contains {"field": "username", "code": "username-too-short", "msg": "username too short"} if { count(input.username) == 0 } -violation contains {"field": "username", "msg": "username too long"} if { +violation contains {"field": "username", "code": "username-too-long", "msg": "username too long"} if { user_id := mxid(input.username, data.server_name) count(user_id) > 255 } -violation contains {"field": "username", "msg": "username contains invalid characters"} if { +violation contains { + "field": "username", "code": "username-all-numeric", + "msg": "username must contain at least one non-numeric character", +} if { + regex.match(`^[0-9]+$`, input.username) +} + +violation contains { + "field": "username", "code": "username-invalid-chars", + "msg": "username contains invalid characters", +} if { not regex.match(`^[a-z0-9.=_/-]+$`, input.username) } diff --git a/policies/register/register_test.rego b/policies/register/register_test.rego index 0199e2ad0..26e119248 100644 --- a/policies/register/register_test.rego +++ b/policies/register/register_test.rego @@ -70,3 +70,7 @@ test_long_username if { test_invalid_username if { not register.allow with input as {"username": "hello world", "registration_method": "upstream-oauth2"} } + +test_numeric_username if { + not register.allow with input as {"username": "1234", "registration_method": "upstream-oauth2"} +} diff --git a/templates/components/field.html b/templates/components/field.html index 10704d90c..c698c5152 100644 --- a/templates/components/field.html +++ b/templates/components/field.html @@ -61,7 +61,21 @@ {% elif error.kind == "exists" and field.name == "username" %} {{ _("mas.errors.username_taken") }} {% elif error.kind == "policy" %} - {{ _("mas.errors.denied_policy", policy=error.message) }} + {% if error.code == "username-too-short" %} + {{ _("mas.errors.username_too_short") }} + {% elif error.code == "username-too-long" %} + {{ _("mas.errors.username_too_long") }} + {% elif error.code == "username-invalid-chars" %} + {{ _("mas.errors.username_invalid_chars") }} + {% elif error.code == "username-all-numeric" %} + {{ _("mas.errors.username_all_numeric") }} + {% elif error.code == "email-domain-not-allowed" %} + {{ _("mas.errors.email_domain_not_allowed") }} + {% elif error.code == "email-domain-banned" %} + {{ _("mas.errors.email_domain_banned") }} + {% else %} + {{ _("mas.errors.denied_policy", policy=error.message) }} + {% endif %} {% elif error.kind == "password_mismatch" %} {{ _("mas.errors.password_mismatch") }} {% else %} diff --git a/translations/en.json b/translations/en.json index 4404ca4ca..e52d4f6db 100644 --- a/translations/en.json +++ b/translations/en.json @@ -284,7 +284,15 @@ }, "denied_policy": "Denied by policy: %(policy)s", "@denied_policy": { - "context": "components/errors.html:17:7-58, components/field.html:64:17-68" + "context": "components/errors.html:17:7-58, components/field.html:77:19-70" + }, + "email_domain_banned": "Email domain is banned by the server policy", + "@email_domain_banned": { + "context": "components/field.html:75:19-54" + }, + "email_domain_not_allowed": "Email domain is not allowed by the server policy", + "@email_domain_not_allowed": { + "context": "components/field.html:73:19-59" }, "field_required": "This field is required", "@field_required": { @@ -296,15 +304,31 @@ }, "password_mismatch": "Password fields don't match", "@password_mismatch": { - "context": "components/errors.html:13:7-40, components/field.html:66:17-50" + "context": "components/errors.html:13:7-40, components/field.html:80:17-50" }, "rate_limit_exceeded": "You've made too many requests in a short period. Please wait a few minutes and try again.", "@rate_limit_exceeded": { "context": "components/errors.html:15:7-42, pages/recovery/progress.html:26:11-46" }, + "username_all_numeric": "Username cannot consist solely of numbers", + "@username_all_numeric": { + "context": "components/field.html:71:19-55" + }, + "username_invalid_chars": "Username contains invalid characters. Use lowercase letters, numbers, dashes and underscores only.", + "@username_invalid_chars": { + "context": "components/field.html:69:19-57" + }, "username_taken": "This username is already taken", "@username_taken": { "context": "components/field.html:62:17-47" + }, + "username_too_long": "Username is too long", + "@username_too_long": { + "context": "components/field.html:67:19-52" + }, + "username_too_short": "Username is too short", + "@username_too_short": { + "context": "components/field.html:65:19-53" } }, "login": { @@ -377,7 +401,7 @@ }, "or_separator": "Or", "@or_separator": { - "context": "components/field.html:85:10-31", + "context": "components/field.html:99:10-31", "description": "Separator between the login methods" }, "policy_violation": {