From 6426dc51e63691e3f7401cb91155112c793d0217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85berg?= Date: Mon, 14 Apr 2025 15:36:14 +0200 Subject: [PATCH] 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. --- .../objc/src/native/autofill/commands/sync.m | 79 ++++++++++++------- .../src/app/services/services.module.ts | 1 + .../services/desktop-autofill.service.ts | 44 +++++++++-- 3 files changed, 86 insertions(+), 38 deletions(-) diff --git a/apps/desktop/desktop_native/objc/src/native/autofill/commands/sync.m b/apps/desktop/desktop_native/objc/src/native/autofill/commands/sync.m index fc13c04591a..34b8bf6d6b0 100644 --- a/apps/desktop/desktop_native/objc/src/native/autofill/commands/sync.m +++ b/apps/desktop/desktop_native/objc/src/native/autofill/commands/sync.m @@ -14,40 +14,59 @@ void runSync(void* context, NSDictionary *params) { // Map credentials to ASPasswordCredential objects NSMutableArray *mappedCredentials = [NSMutableArray arrayWithCapacity:credentials.count]; + for (NSDictionary *credential in credentials) { - NSString *type = credential[@"type"]; - - if ([type isEqualToString:@"password"]) { - NSString *cipherId = credential[@"cipherId"]; - 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]; - } - - if (@available(macos 14, *)) { - if ([type isEqualToString:@"fido2"]) { + @try { + NSString *type = credential[@"type"]; + + if ([type isEqualToString:@"password"]) { NSString *cipherId = credential[@"cipherId"]; - NSString *rpId = credential[@"rpId"]; - NSString *userName = credential[@"userName"]; - NSData *credentialId = decodeBase64URL(credential[@"credentialId"]); - NSData *userHandle = decodeBase64URL(credential[@"userHandle"]); + NSString *uri = credential[@"uri"]; + 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; + } - Class passkeyCredentialIdentityClass = NSClassFromString(@"ASPasskeyCredentialIdentity"); - id credential = [[passkeyCredentialIdentityClass alloc] - initWithRelyingPartyIdentifier:rpId - userName:userName - credentialID:credentialId - userHandle:userHandle - recordIdentifier:cipherId]; + ASCredentialServiceIdentifier *serviceId = [[ASCredentialServiceIdentifier alloc] + initWithIdentifier:uri type:ASCredentialServiceIdentifierTypeURL]; + ASPasswordCredentialIdentity *passwordIdentity = [[ASPasswordCredentialIdentity alloc] + initWithServiceIdentifier:serviceId user:username recordIdentifier:cipherId]; - [mappedCredentials addObject:credential]; + [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 passkeyIdentity = [[passkeyCredentialIdentityClass alloc] + initWithRelyingPartyIdentifier:rpId + userName:userName + credentialID:credentialId + userHandle:userHandle + recordIdentifier:cipherId]; + + [mappedCredentials addObject:passkeyIdentity]; + } } + } @catch (NSException *exception) { + // Silently skip any credential that causes an exception + NSLog(@"ERROR: Exception processing credential: %@ - %@", exception.name, exception.reason); + continue; } } @@ -59,4 +78,4 @@ void runSync(void* context, NSDictionary *params) { _return(context, _success(@{@"added": @([mappedCredentials count])})); }]; -} +} \ No newline at end of file diff --git a/apps/desktop/src/app/services/services.module.ts b/apps/desktop/src/app/services/services.module.ts index db28f01f27a..78673431f4e 100644 --- a/apps/desktop/src/app/services/services.module.ts +++ b/apps/desktop/src/app/services/services.module.ts @@ -332,6 +332,7 @@ const safeProviders: SafeProvider[] = [ ConfigService, Fido2AuthenticatorServiceAbstraction, AccountService, + AuthService, ], }), safeProvider({ diff --git a/apps/desktop/src/autofill/services/desktop-autofill.service.ts b/apps/desktop/src/autofill/services/desktop-autofill.service.ts index 7fcc3425082..3b1f7d3605f 100644 --- a/apps/desktop/src/autofill/services/desktop-autofill.service.ts +++ b/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, 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( - map((account) => account?.id), - filter((userId): userId is UserId => userId != null), - switchMap((userId) => this.cipherService.cipherViews$(userId)), + //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), + ), + 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({ namespace: "autofill", command: "sync",