Improve MacOS Syncing

This changes the behaviour to react to logoff, but not to account locks. It also adds better error handling on the native side.

apps/desktop/desktop_native/objc/src/native/autofill/commands/sync.m

@@ -14,7 +14,9 @@ void runSync(void* context, NSDictionary *params) {
 
   // Map credentials to ASPasswordCredential objects
   NSMutableArray *mappedCredentials = [NSMutableArray arrayWithCapacity:credentials.count];
+  
   for (NSDictionary *credential in credentials) {
+    @try {
       NSString *type = credential[@"type"];
       
       if ([type isEqualToString:@"password"]) {
@@ -22,33 +24,50 @@ void runSync(void* context, NSDictionary *params) {
         NSString *uri = credential[@"uri"];
         NSString *username = credential[@"username"];
         
-      ASCredentialServiceIdentifier *serviceId = [[ASCredentialServiceIdentifier alloc]
-        initWithIdentifier:uri type:ASCredentialServiceIdentifierTypeURL];
-      ASPasswordCredentialIdentity *credential = [[ASPasswordCredentialIdentity alloc]
-        initWithServiceIdentifier:serviceId user:username recordIdentifier:cipherId];
-
-      [mappedCredentials addObject:credential];
+        // Skip credentials with null username
+        if ([username isKindOfClass:[NSNull class]] || username.length == 0) {
+            NSLog(@"Skipping credential, username is empty: %@", credential);
+          continue;
         }
 
-    if (@available(macos 14, *)) {
+        ASCredentialServiceIdentifier *serviceId = [[ASCredentialServiceIdentifier alloc]
+          initWithIdentifier:uri type:ASCredentialServiceIdentifierTypeURL];
+        ASPasswordCredentialIdentity *passwordIdentity = [[ASPasswordCredentialIdentity alloc]
+          initWithServiceIdentifier:serviceId user:username recordIdentifier:cipherId];
+
+        [mappedCredentials addObject:passwordIdentity];
+      } 
+      else if (@available(macos 14, *)) {
         if ([type isEqualToString:@"fido2"]) {
           NSString *cipherId = credential[@"cipherId"];
           NSString *rpId = credential[@"rpId"];
           NSString *userName = credential[@"userName"];
+          
+          // Skip credentials with null userName
+          if ([userName isKindOfClass:[NSNull class]] || userName.length == 0) {
+            NSLog(@"Skipping credential, username is empty: %@", credential);
+            continue;
+          }
+          
           NSData *credentialId = decodeBase64URL(credential[@"credentialId"]);
           NSData *userHandle = decodeBase64URL(credential[@"userHandle"]);
           
           Class passkeyCredentialIdentityClass = NSClassFromString(@"ASPasskeyCredentialIdentity");
-        id credential = [[passkeyCredentialIdentityClass alloc]
+          id passkeyIdentity = [[passkeyCredentialIdentityClass alloc]
             initWithRelyingPartyIdentifier:rpId
             userName:userName
             credentialID:credentialId
             userHandle:userHandle
             recordIdentifier:cipherId];
 
-        [mappedCredentials addObject:credential];
+          [mappedCredentials addObject:passkeyIdentity];
         }
       }
+    } @catch (NSException *exception) {
+      // Silently skip any credential that causes an exception
+      NSLog(@"ERROR: Exception processing credential: %@ - %@", exception.name, exception.reason);
+      continue;
+    }
   }
 
   [ASCredentialIdentityStore.sharedStore replaceCredentialIdentityEntries:mappedCredentials

apps/desktop/src/app/services/services.module.ts

@@ -332,6 +332,7 @@ const safeProviders: SafeProvider[] = [
       ConfigService,
       Fido2AuthenticatorServiceAbstraction,
       AccountService,
+      AuthService,
     ],
   }),
   safeProvider({

apps/desktop/src/autofill/services/desktop-autofill.service.ts

@@ -2,6 +2,8 @@ import { Injectable, OnDestroy } from "@angular/core";
 import { autofill } from "desktop_native/napi";
 import {
   Subject,
+  combineLatest,
+  debounceTime,
   distinctUntilChanged,
   filter,
   firstValueFrom,
@@ -12,6 +14,8 @@ import {
 } from "rxjs";
 
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
+import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
 import { getOptionalUserId } from "@bitwarden/common/auth/services/account.service";
 import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
 import { UriMatchStrategy } from "@bitwarden/common/models/domain/domain-service";
@@ -52,6 +56,7 @@ export class DesktopAutofillService implements OnDestroy {
     private configService: ConfigService,
     private fido2AuthenticatorService: Fido2AuthenticatorServiceAbstraction<NativeWindowObject>,
     private accountService: AccountService,
+    private authService: AuthService,
   ) {}
 
   async init() {
@@ -59,21 +64,39 @@ export class DesktopAutofillService implements OnDestroy {
       .getFeatureFlag$(FeatureFlag.MacOsNativeCredentialSync)
       .pipe(
         distinctUntilChanged(),
-        switchMap((enabled) => {
-          return this.accountService.activeAccount$.pipe(
+        //filter((enabled) => enabled === true), // Only proceed if feature is enabled
+        switchMap(() => {
+          return combineLatest([
+            this.accountService.activeAccount$.pipe(
               map((account) => account?.id),
               filter((userId): userId is UserId => userId != null),
-            switchMap((userId) => this.cipherService.cipherViews$(userId)),
+            ),
+            this.authService.activeAccountStatus$,
+          ]).pipe(
+            // Only proceed when the vault is unlocked
+            filter(([, status]) => status === AuthenticationStatus.Unlocked),
+            // Then get cipher views
+            switchMap(([userId]) => this.cipherService.cipherViews$(userId)),
           );
         }),
-        // TODO: This will unset all the autofill credentials on the OS
-        // when the account locks. We should instead explicilty clear the credentials
-        // when the user logs out. Maybe by subscribing to the encrypted ciphers observable instead.
+        debounceTime(100), // just a precaution to not spam the sync if there are multiple changes (we typically observe a null change)
+        // No filter for empty arrays here - we want to sync even if there are 0 items
+        filter((cipherViewMap) => cipherViewMap !== null),
+
         mergeMap((cipherViewMap) => this.sync(Object.values(cipherViewMap ?? []))),
         takeUntil(this.destroy$),
       )
       .subscribe();
 
+    // Listen for sign out to clear credentials?
+    this.authService.activeAccountStatus$
+      .pipe(
+        filter((status) => status === AuthenticationStatus.LoggedOut),
+        mergeMap(() => this.sync([])), // sync an empty array
+        takeUntil(this.destroy$),
+      )
+      .subscribe();
+
     this.listenIpc();
   }
 
@@ -131,6 +154,11 @@ export class DesktopAutofillService implements OnDestroy {
       }));
     }
 
+    this.logService.warning("Syncing autofill credentials", {
+      fido2Credentials,
+      passwordCredentials,
+    });
+
     const syncResult = await ipc.autofill.runCommand<NativeAutofillSyncCommand>({
       namespace: "autofill",
       command: "sync",