Don't apply a session limit when genuinely replacing a session

Author: reivilibre

crates/handlers/src/compat/login.rs

@@ -576,7 +576,8 @@ async fn token_login(
         Device::generate(rng)
     };
 
-    repo.app_session()
+    let session_replaced = repo
+        .app_session()
         .finish_sessions_to_replace_device(clock, &browser_session.user, &device)
         .await?;
 
@@ -586,6 +587,7 @@ async fn token_login(
         .evaluate_compat_login(mas_policy::CompatLoginInput {
             user: &browser_session.user,
             login_type: CompatLoginType::WebSso,
+            session_replaced,
             session_counts,
             requester,
         })
@@ -702,7 +704,8 @@ async fn user_password_login(
         Device::generate(&mut rng)
     };
 
-    repo.app_session()
+    let session_replaced = repo
+        .app_session()
         .finish_sessions_to_replace_device(clock, &user, &device)
         .await?;
 
@@ -712,6 +715,7 @@ async fn user_password_login(
         .evaluate_compat_login(mas_policy::CompatLoginInput {
             user: &user,
             login_type: CompatLoginType::Password,
+            session_replaced,
             session_counts,
             requester: policy_requester,
         })

crates/handlers/src/compat/login_sso_complete.rs

@@ -123,6 +123,8 @@ pub async fn get(
         .evaluate_compat_login(mas_policy::CompatLoginInput {
             user: &session.user,
             login_type: CompatLoginType::WebSso,
+            // TODO should we predict a replacement?
+            session_replaced: false,
             session_counts,
             requester: mas_policy::Requester {
                 ip_address: activity_tracker.ip(),
@@ -251,6 +253,8 @@ pub async fn post(
             user: &session.user,
             login_type: CompatLoginType::WebSso,
             session_counts,
+            // TODO should we predict a replacement?
+            session_replaced: false,
             requester: mas_policy::Requester {
                 ip_address: activity_tracker.ip(),
                 user_agent,

crates/policy/src/model.rs

@@ -197,6 +197,9 @@ pub struct CompatLoginInput<'a> {
     /// How many sessions the user has.
     pub session_counts: SessionCounts,
 
+    /// Whether a session will be replaced by this login
+    pub session_replaced: bool,
+
     // TODO is this actually what we care about? Don't we care a bit more about whether we're in an
     // interactive context or a non-interactive context? (SSO type has both phases :()
     pub login_type: CompatLoginType,

policies/compat_login/compat_login.rego

@@ -36,6 +36,10 @@ violation contains {
 	# TODO not strictly correct...
 	input.login_type == "m.login.sso"
 
+	# Only apply if this login doesn't replace a session
+	# (As then this login is not actually increasing the number of devices)
+	not input.session_replaced
+
 	# For web-based 'compat SSO' login, a violation occurs when the soft limit has already been
 	# reached or exceeded.
 	# We use the soft limit because the user will be able to interactively remove
@@ -53,6 +57,10 @@ violation contains {
 	# This is not a web-based interactive login
 	input.login_type == "m.login.password"
 
+	# Only apply if this login doesn't replace a session
+	# (As then this login is not actually increasing the number of devices)
+	not input.session_replaced
+
 	# For `m.login.password` login, a violation occurs when the hard limit has already been
 	# reached or exceeded.
 	# We don't use the soft limit because the user won't be able to interactively remove

policies/compat_login/compat_login_test.rego

@@ -14,59 +14,84 @@ test_session_limiting_sso if {
 	compat_login.allow with input.user as user
 		with input.session_counts as {"total": 1}
 		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	compat_login.allow with input.user as user
 		with input.session_counts as {"total": 31}
 		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	not compat_login.allow with input.user as user
 		with input.session_counts as {"total": 32}
 		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	not compat_login.allow with input.user as user
 		with input.session_counts as {"total": 42}
 		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	not compat_login.allow with input.user as user
 		with input.session_counts as {"total": 65}
 		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	# No limit configured
 	compat_login.allow with input.user as user
 		with input.session_counts as {"total": 1}
 		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
 		with data.session_limit as null
 }
 
 test_session_limiting_password if {
 	compat_login.allow with input.user as user
 		with input.session_counts as {"total": 1}
 		with input.login_type as "m.login.password"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	compat_login.allow with input.user as user
 		with input.session_counts as {"total": 63}
 		with input.login_type as "m.login.password"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	not compat_login.allow with input.user as user
 		with input.session_counts as {"total": 64}
 		with input.login_type as "m.login.password"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
     not compat_login.allow with input.user as user
 		with input.session_counts as {"total": 65}
 		with input.login_type as "m.login.password"
+		with input.session_replaced as false
 		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
 
 	# No limit configured
 	compat_login.allow with input.user as user
 		with input.session_counts as {"total": 1}
 		with input.login_type as "m.login.password"
+		with input.session_replaced as false
 		with data.session_limit as null
 }
+
+test_no_session_limiting_upon_replacement if {
+    not compat_login.allow with input.user as user
+		with input.session_counts as {"total": 65}
+		with input.login_type as "m.login.password"
+		with input.session_replaced as false
+		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
+
+	not compat_login.allow with input.user as user
+		with input.session_counts as {"total": 65}
+		with input.login_type as "m.login.sso"
+		with input.session_replaced as false
+		with data.session_limit as {"soft_limit": 32, "hard_limit": 64}
+}

policies/schema/compat_login_input.json

@@ -7,6 +7,7 @@
     "login_type",
     "requester",
     "session_counts",
+    "session_replaced",
     "user"
   ],
   "properties": {
@@ -22,6 +23,10 @@
         }
       ]
     },
+    "session_replaced": {
+      "description": "Whether a session will be replaced by this login",
+      "type": "boolean"
+    },
     "login_type": {
       "$ref": "#/definitions/CompatLoginType"
     },