Fence login session commits

Signed-off-by: coder 1 <a93f3b1decd199cec83848f116ff60c20776cdd03861b9bba2610acae7c9eeeb@buzz.block.builderlab.xyz>
Signed-off-by: coder 0 <d97ebdbb198c7237c94f84ea8bb8a73583ea067407eebd0062abbb3962527fb1@buzz.block.builderlab.xyz>
Signed-off-by: bb-expert <51cde9442e46eac81c83ec71552708c3b0cb96a88ff8e1581d8fb4de281afbf3@buzz.block.builderlab.xyz>

desktop/src-tauri/src/builderlab.rs

@@ -299,6 +299,40 @@ fn register_pending_builderlab_login(
     Ok(true)
 }
 
+fn commit_builderlab_login_session(
+    login: &BuilderlabLogin,
+    session: &BuilderlabSession,
+    login_id: &str,
+    credential: String,
+) -> Result<(), String> {
+    commit_builderlab_login_session_with_hook(login, session, login_id, credential, || {})
+}
+
+fn commit_builderlab_login_session_with_hook(
+    login: &BuilderlabLogin,
+    session: &BuilderlabSession,
+    login_id: &str,
+    credential: String,
+    before_session_lock: impl FnOnce(),
+) -> Result<(), String> {
+    // Keep this lock order consistent everywhere a Builderlab login result is
+    // committed: first BuilderlabLogin, then BuilderlabSession. Holding the login
+    // lock through the session write prevents cancel/replacement from
+    // interleaving after the ownership check but before credential storage.
+    let mut login_state = login.0.lock().map_err(|error| error.to_string())?;
+    if login_state
+        .pending
+        .as_ref()
+        .is_none_or(|pending| pending.id != login_id)
+    {
+        return Err("Builderlab authentication canceled".to_owned());
+    }
+    before_session_lock();
+    *session.0.lock().map_err(|error| error.to_string())? = Some(StoredSession { credential });
+    login_state.pending = None;
+    Ok(())
+}
+
 async fn abort_builderlab_login(
     login: &BuilderlabLogin,
     server: tokio::task::JoinHandle<()>,
@@ -489,20 +523,7 @@ pub(crate) async fn start_builderlab_login(
         email: me.email,
         name: me.name,
     };
-    {
-        let mut state = login.0.lock().map_err(|error| error.to_string())?;
-        if state
-            .pending
-            .as_ref()
-            .is_none_or(|pending| pending.id != login_id)
-        {
-            return Err("Builderlab authentication canceled".to_owned());
-        }
-        state.pending = None;
-    }
-    *session.0.lock().map_err(|error| error.to_string())? = Some(StoredSession {
-        credential: exchanged.session_credential,
-    });
+    commit_builderlab_login_session(&login, &session, &login_id, exchanged.session_credential)?;
     Ok(info)
 }
 
@@ -855,6 +876,92 @@ mod tests {
         );
     }
 
+    #[test]
+    fn canceled_attempt_tombstones_are_bounded() {
+        let login = BuilderlabLogin::default();
+        for index in 0..(CANCELED_LOGIN_ATTEMPT_TOMBSTONE_LIMIT + 5) {
+            let attempt_id = format!("enterprise-login-{index}");
+            assert!(!cancel_builderlab_login_attempt(&login, Some(&attempt_id)).unwrap());
+        }
+
+        let state = login.0.lock().unwrap();
+        assert_eq!(
+            state.canceled_attempts.len(),
+            CANCELED_LOGIN_ATTEMPT_TOMBSTONE_LIMIT,
+        );
+        assert_eq!(
+            state.canceled_attempts.front().map(String::as_str),
+            Some("enterprise-login-5"),
+        );
+    }
+
+    #[test]
+    fn stale_attempt_cannot_commit_session_after_replacement() {
+        let login = BuilderlabLogin::default();
+        let session = BuilderlabSession::default();
+        let (cancel, _receiver) = oneshot::channel();
+        login.0.lock().unwrap().pending = Some(PendingLogin {
+            id: "enterprise-login-new".to_owned(),
+            cancel,
+        });
+
+        let result = commit_builderlab_login_session(
+            &login,
+            &session,
+            "enterprise-login-old",
+            "stale-credential".to_owned(),
+        );
+
+        assert_eq!(result.unwrap_err(), "Builderlab authentication canceled");
+        assert!(session.0.lock().unwrap().is_none());
+        assert_eq!(
+            login
+                .0
+                .lock()
+                .unwrap()
+                .pending
+                .as_ref()
+                .map(|pending| pending.id.as_str()),
+            Some("enterprise-login-new"),
+        );
+    }
+
+    #[test]
+    fn session_commit_holds_login_ownership_fence_through_write() {
+        let login = BuilderlabLogin::default();
+        let session = BuilderlabSession::default();
+        let (cancel, _receiver) = oneshot::channel();
+        login.0.lock().unwrap().pending = Some(PendingLogin {
+            id: "enterprise-login-a".to_owned(),
+            cancel,
+        });
+
+        commit_builderlab_login_session_with_hook(
+            &login,
+            &session,
+            "enterprise-login-a",
+            "fresh-credential".to_owned(),
+            || {
+                assert!(
+                    login.0.try_lock().is_err(),
+                    "login ownership must remain locked until after the session write",
+                );
+            },
+        )
+        .unwrap();
+
+        assert!(login.0.lock().unwrap().pending.is_none());
+        assert_eq!(
+            session
+                .0
+                .lock()
+                .unwrap()
+                .as_ref()
+                .map(|stored| stored.credential.as_str()),
+            Some("fresh-credential"),
+        );
+    }
+
     #[test]
     fn cancel_without_attempt_id_preserves_existing_blind_cancel_behavior() {
         let login = BuilderlabLogin::default();