Propagate more specific error messages from the policy on registration

This makes some policy errors translatable

Author: sandhose

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,
                                 }),
                             }

crates/handlers/src/views/register.rs

@@ -154,20 +154,22 @@ 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? {
             // The user already exists in the database
             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) {

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)]

crates/policy/src/model.rs

@@ -13,13 +13,53 @@ 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))]
 pub struct Violation {
     pub msg: String,
     pub redirect_uri: Option<String>,
     pub field: Option<String>,
+    pub code: Option<Code>,
 }
 
 /// The result of a policy evaluation.

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(),
                         },
                     ),

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,
     },

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)

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)
 }
 

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"}
+}

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 %}