From 1afd2a2906c64e5f264525316356013ec4aa5684 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Tue, 16 Jul 2024 14:33:04 +0100 Subject: [PATCH] Remove OPA-based password policy enforcement (#2875) Co-authored-by: Quentin Gliech --- crates/cli/src/util.rs | 1 - crates/handlers/src/graphql/mutations/user.rs | 6 +-- crates/handlers/src/test_utils.rs | 1 - crates/handlers/src/views/recovery/finish.rs | 13 ------- crates/handlers/src/views/register.rs | 2 +- crates/policy/src/lib.rs | 37 +++---------------- crates/policy/src/model.rs | 6 +-- policies/Makefile | 2 - policies/password.rego | 30 --------------- policies/password_test.rego | 29 --------------- policies/register.rego | 9 ----- policies/register_test.rego | 37 ------------------- policies/schema/register_input.json | 4 -- 13 files changed, 9 insertions(+), 168 deletions(-) delete mode 100644 policies/password.rego delete mode 100644 policies/password_test.rego diff --git a/crates/cli/src/util.rs b/crates/cli/src/util.rs index 0ca1024d..a91d2bf3 100644 --- a/crates/cli/src/util.rs +++ b/crates/cli/src/util.rs @@ -112,7 +112,6 @@ pub async fn policy_factory_from_config( client_registration: config.client_registration_entrypoint.clone(), authorization_grant: config.authorization_grant_entrypoint.clone(), email: config.email_entrypoint.clone(), - password: config.password_entrypoint.clone(), }; PolicyFactory::load(policy_file, config.data.clone(), entrypoints) diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index 1fa64c6c..bd3dd305 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.rs @@ -546,11 +546,7 @@ impl UserMutations { return Err(async_graphql::Error::new("Unauthorized")); } - let mut policy = state.policy().await?; - - let res = policy.evaluate_password(&input.new_password).await?; - - if !res.valid() { + if input.new_password.is_empty() { // TODO Expose the reason for the policy violation // This involves redesigning the error handling // Idea would be to expose an errors array in the response, diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index f2870870..8d7ea37c 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -85,7 +85,6 @@ pub(crate) async fn policy_factory( client_registration: "client_registration/violation".to_owned(), authorization_grant: "authorization_grant/violation".to_owned(), email: "email/violation".to_owned(), - password: "password/violation".to_owned(), }; let policy_factory = PolicyFactory::load(file, data, entrypoints).await?; diff --git a/crates/handlers/src/views/recovery/finish.rs b/crates/handlers/src/views/recovery/finish.rs index 0086100a..9881f8ac 100644 --- a/crates/handlers/src/views/recovery/finish.rs +++ b/crates/handlers/src/views/recovery/finish.rs @@ -24,7 +24,6 @@ use mas_axum_utils::{ FancyError, }; use mas_data_model::SiteConfig; -use mas_policy::Policy; use mas_router::UrlBuilder; use mas_storage::{BoxClock, BoxRepository, BoxRng}; use mas_templates::{ @@ -129,7 +128,6 @@ pub(crate) async fn post( mut rng: BoxRng, clock: BoxClock, mut repo: BoxRepository, - mut policy: Policy, State(site_config): State, State(password_manager): State, State(templates): State, @@ -238,17 +236,6 @@ pub(crate) async fn post( ); } - let res = policy.evaluate_password(&form.new_password).await?; - - if !res.valid() { - form_state = form_state.with_error_on_field( - RecoveryFinishFormField::NewPassword, - FieldError::Policy { - message: res.to_string(), - }, - ); - } - if !form_state.is_valid() { let context = RecoveryFinishContext::new(user) .with_form_state(form_state) diff --git a/crates/handlers/src/views/register.rs b/crates/handlers/src/views/register.rs index 7049f6fd..495aa86c 100644 --- a/crates/handlers/src/views/register.rs +++ b/crates/handlers/src/views/register.rs @@ -214,7 +214,7 @@ pub(crate) async fn post( } let res = policy - .evaluate_register(&form.username, &form.password, &form.email) + .evaluate_register(&form.username, &form.email) .await?; for violation in res.violations { diff --git a/crates/policy/src/lib.rs b/crates/policy/src/lib.rs index 00e8f5a5..51b9dcc6 100644 --- a/crates/policy/src/lib.rs +++ b/crates/policy/src/lib.rs @@ -23,9 +23,7 @@ use opa_wasm::{ use thiserror::Error; use tokio::io::{AsyncRead, AsyncReadExt}; -use self::model::{ - AuthorizationGrantInput, ClientRegistrationInput, EmailInput, PasswordInput, RegisterInput, -}; +use self::model::{AuthorizationGrantInput, ClientRegistrationInput, EmailInput, RegisterInput}; pub use self::model::{EvaluationResult, Violation}; use crate::model::GrantType; @@ -66,17 +64,15 @@ pub struct Entrypoints { pub client_registration: String, pub authorization_grant: String, pub email: String, - pub password: String, } impl Entrypoints { - fn all(&self) -> [&str; 5] { + fn all(&self) -> [&str; 4] { [ self.register.as_str(), self.client_registration.as_str(), self.authorization_grant.as_str(), self.email.as_str(), - self.password.as_str(), ] } } @@ -195,21 +191,6 @@ impl Policy { Ok(res) } - #[tracing::instrument(name = "policy.evaluate_password", skip_all, err)] - pub async fn evaluate_password( - &mut self, - password: &str, - ) -> Result { - let input = PasswordInput { password }; - - let [res]: [EvaluationResult; 1] = self - .instance - .evaluate(&mut self.store, &self.entrypoints.password, &input) - .await?; - - Ok(res) - } - #[tracing::instrument( name = "policy.evaluate.register", skip_all, @@ -223,14 +204,9 @@ impl Policy { pub async fn evaluate_register( &mut self, username: &str, - password: &str, email: &str, ) -> Result { - let input = RegisterInput::Password { - username, - password, - email, - }; + let input = RegisterInput::Password { username, email }; let [res]: [EvaluationResult; 1] = self .instance @@ -415,7 +391,6 @@ mod tests { client_registration: "client_registration/violation".to_owned(), authorization_grant: "authorization_grant/violation".to_owned(), email: "email/violation".to_owned(), - password: "password/violation".to_owned(), }; let factory = PolicyFactory::load(file, data, entrypoints).await.unwrap(); @@ -423,19 +398,19 @@ mod tests { let mut policy = factory.instantiate().await.unwrap(); let res = policy - .evaluate_register("hello", "hunter2", "hello@example.com") + .evaluate_register("hello", "hello@example.com") .await .unwrap(); assert!(!res.valid()); let res = policy - .evaluate_register("hello", "hunter2", "hello@foo.element.io") + .evaluate_register("hello", "hello@foo.element.io") .await .unwrap(); assert!(res.valid()); let res = policy - .evaluate_register("hello", "hunter2", "hello@staging.element.io") + .evaluate_register("hello", "hello@staging.element.io") .await .unwrap(); assert!(!res.valid()); diff --git a/crates/policy/src/model.rs b/crates/policy/src/model.rs index ef09723c..2872e59c 100644 --- a/crates/policy/src/model.rs +++ b/crates/policy/src/model.rs @@ -65,11 +65,7 @@ impl EvaluationResult { #[cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema))] pub enum RegisterInput<'a> { #[serde(rename = "password")] - Password { - username: &'a str, - password: &'a str, - email: &'a str, - }, + Password { username: &'a str, email: &'a str }, #[serde(rename = "upstream-oauth2")] UpstreamOAuth2 { diff --git a/policies/Makefile b/policies/Makefile index d216fc01..291304a6 100644 --- a/policies/Makefile +++ b/policies/Makefile @@ -7,7 +7,6 @@ INPUTS := \ client_registration.rego \ register.rego \ authorization_grant.rego \ - password.rego \ email.rego ifeq ($(DOCKER), 1) @@ -27,7 +26,6 @@ policy.wasm: $(INPUTS) -e "client_registration/violation" \ -e "register/violation" \ -e "authorization_grant/violation" \ - -e "password/violation" \ -e "email/violation" \ $^ tar xzf bundle.tar.gz /policy.wasm diff --git a/policies/password.rego b/policies/password.rego deleted file mode 100644 index bae1c215..00000000 --- a/policies/password.rego +++ /dev/null @@ -1,30 +0,0 @@ -# METADATA -# schemas: -# - input: schema["password_input"] -package password - -default allow := false - -allow { - count(violation) == 0 -} - -violation[{"msg": msg}] { - count(input.password) < data.passwords.min_length - msg := sprintf("needs to be at least %d characters", [data.passwords.min_length]) -} - -violation[{"msg": "requires at least one number"}] { - data.passwords.require_number - not regex.match("[0-9]", input.password) -} - -violation[{"msg": "requires at least one lowercase letter"}] { - data.passwords.require_lowercase - not regex.match("[a-z]", input.password) -} - -violation[{"msg": "requires at least one uppercase letter"}] { - data.passwords.require_uppercase - not regex.match("[A-Z]", input.password) -} diff --git a/policies/password_test.rego b/policies/password_test.rego deleted file mode 100644 index 4748974d..00000000 --- a/policies/password_test.rego +++ /dev/null @@ -1,29 +0,0 @@ -package password - -test_password_require_number { - allow with data.passwords.require_number as true - - not allow with input.password as "hunter" - with data.passwords.require_number as true -} - -test_password_require_lowercase { - allow with data.passwords.require_lowercase as true - - not allow with input.password as "HUNTER2" - with data.passwords.require_lowercase as true -} - -test_password_require_uppercase { - allow with data.passwords.require_uppercase as true - - not allow with input.password as "hunter2" - with data.passwords.require_uppercase as true -} - -test_password_min_length { - allow with data.passwords.min_length as 6 - - not allow with input.password as "short" - with data.passwords.min_length as 6 -} diff --git a/policies/register.rego b/policies/register.rego index 61230882..05193549 100644 --- a/policies/register.rego +++ b/policies/register.rego @@ -4,7 +4,6 @@ package register import data.email as email_policy -import data.password as password_policy import future.keywords.in @@ -34,14 +33,6 @@ violation[{"msg": "unknown registration method"}] { not input.registration_method in ["password", "upstream-oauth2"] } -violation[object.union({"field": "password"}, v)] { - # Check if the registration method is password - input.registration_method == "password" - - # Get the violation object from the password policy - some v in password_policy.violation -} - # Check that we supplied an email for password registration violation[{"field": "email", "msg": "email required for password-based registration"}] { input.registration_method == "password" diff --git a/policies/register_test.rego b/policies/register_test.rego index 6d1293fd..4d8d5476 100644 --- a/policies/register_test.rego +++ b/policies/register_test.rego @@ -3,7 +3,6 @@ package register mock_registration := { "registration_method": "password", "username": "hello", - "password": "Hunter2", "email": "hello@staging.element.io", } @@ -51,39 +50,3 @@ test_long_username { test_invalid_username { not allow with input as {"username": "hello world", "registration_method": "upstream-oauth2"} } - -test_password_require_number { - allow with input as mock_registration - with data.passwords.require_number as true - - not allow with input as mock_registration - with input.password as "hunter" - with data.passwords.require_number as true -} - -test_password_require_lowercase { - allow with input as mock_registration - with data.passwords.require_lowercase as true - - not allow with input as mock_registration - with input.password as "HUNTER2" - with data.passwords.require_lowercase as true -} - -test_password_require_uppercase { - allow with input as mock_registration - with data.passwords.require_uppercase as true - - not allow with input as mock_registration - with input.password as "hunter2" - with data.passwords.require_uppercase as true -} - -test_password_min_length { - allow with input as mock_registration - with data.passwords.min_length as 6 - - not allow with input as mock_registration - with input.password as "short" - with data.passwords.min_length as 6 -} diff --git a/policies/schema/register_input.json b/policies/schema/register_input.json index 455accfc..e1d796ea 100644 --- a/policies/schema/register_input.json +++ b/policies/schema/register_input.json @@ -7,7 +7,6 @@ "type": "object", "required": [ "email", - "password", "registration_method", "username" ], @@ -21,9 +20,6 @@ "username": { "type": "string" }, - "password": { - "type": "string" - }, "email": { "type": "string" }