Remove OPA-based password policy enforcement (#2875)

Co-authored-by: Quentin Gliech <quenting@element.io>

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)

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?;
+        if input.new_password.is_empty() {
-
-        let res = policy.evaluate_password(&input.new_password).await?;
-
-        if !res.valid() {
             // 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,

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?;

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<SiteConfig>,
     State(password_manager): State<PasswordManager>,
     State(templates): State<Templates>,
@@ -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)

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 {

crates/policy/src/lib.rs

@@ -23,9 +23,7 @@ use opa_wasm::{
 use thiserror::Error;
 use tokio::io::{AsyncRead, AsyncReadExt};
 
-use self::model::{
+use self::model::{AuthorizationGrantInput, ClientRegistrationInput, EmailInput, RegisterInput};
-    AuthorizationGrantInput, ClientRegistrationInput, EmailInput, PasswordInput, 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<EvaluationResult, EvaluationError> {
-        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<EvaluationResult, EvaluationError> {
-        let input = RegisterInput::Password {
+        let input = RegisterInput::Password { username, email };
-            username,
-            password,
-            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());

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 {
+    Password { username: &'a str, email: &'a str },
-        username: &'a str,
-        password: &'a str,
-        email: &'a str,
-    },
 
     #[serde(rename = "upstream-oauth2")]
     UpstreamOAuth2 {

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

policies/password.rego

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

policies/password_test.rego

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

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"

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

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