letro/letro-authentication-service
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Propagate more specific error messages from the policy on registration

Browse Source 
This makes some policy errors translatable
This commit is contained in:
Quentin Gliech
2024-12-19 15:44:14 +01:00
parent 7f843b8bdc
commit a51ab2fb5c
11 changed files with 137 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
crates/handlers/src/upstream_oauth2/link.rs
View File

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

35
crates/handlers/src/views/register.rs
View File

@@ -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) {

2
crates/policy/src/lib.rs
View File

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

40
crates/policy/src/model.rs
View File

@@ -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<String>,
pub field: Option<String>,
pub code: Option<Code>,
}
/// The result of a policy evaluation.

1
crates/templates/src/context.rs
View File

@@ -462,6 +462,7 @@ impl TemplateContext for LoginContext {
.with_error_on_field(
LoginFormField::Password,
FieldError::Policy {
code: None,
message: "password too short".to_owned(),
},
),

6
crates/templates/src/forms.rs
View File

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

4
policies/email/email.rego
View File

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

16
policies/register/register.rego
View File

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

4
policies/register/register_test.rego
View File

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

16
templates/components/field.html
View File

@@ -61,7 +61,21 @@ Please see LICENSE in the repository root for full details.
{% 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 %}

30
translations/en.json
View File

@@ -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": {