Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(notifications_push_repository): Delete push subscription when account is deleted #2667

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

part 'account_storage.dart';

/// Signature of a callback that executed right before the [account] is logged out.
typedef BeforeLogOutCallback = Future<void> Function(Account account);

/// {@template account_failure}
/// A base failure for the account repository failures.
/// {@endtemplate}
Expand Down Expand Up @@ -86,6 +89,7 @@
final String _userAgent;
final http.Client _httpClient;
final AccountStorage _storage;
final List<BeforeLogOutCallback> _beforeLogOutCallbacks = [];

final BehaviorSubject<({String? active, BuiltMap<String, Account> accounts})> _accounts =
BehaviorSubject.seeded((active: null, accounts: BuiltMap()));
Expand Down Expand Up @@ -272,6 +276,18 @@
]);
}

/// Registers a new [callback] that is executed right before an account is logged out.
///
/// The callback must not throw any exceptions and handle them gracefully itself.
void registerBeforeLogOutCallback(BeforeLogOutCallback callback) {
_beforeLogOutCallbacks.add(callback);
}

/// Unregisters a [callback] that was previously registered with [registerBeforeLogOutCallback].

Check warning on line 286 in packages/neon_framework/packages/account_repository/lib/src/account_repository.dart

View workflow job for this annotation

GitHub Actions / spellcheck

Unknown word (Unregisters)
void unregisterBeforeLogOutCallback(BeforeLogOutCallback callback) {
_beforeLogOutCallbacks.remove(callback);
}

/// Logs out the user from the server.
///
/// May throw a [DeleteCredentialsFailure].
Expand Down Expand Up @@ -299,8 +315,12 @@

_accounts.add((active: active, accounts: accounts));

for (final callback in _beforeLogOutCallbacks) {
await callback(account!);
}
Comment on lines +318 to +320
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that running these callbacks in parallel should be ok.
The account_repo can't guarantee execution order anyway.

Please use a Future.wait


try {
await account?.client.authentication.appPassword.deleteAppPassword();
await account!.client.authentication.appPassword.deleteAppPassword();
} on http.ClientException catch (error, stackTrace) {
Error.throwWithStackTrace(DeleteCredentialsFailure(error), stackTrace);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ class _UsersClientMock extends Mock implements provisioning_api.$UsersClient {}

class _AccountStorageMock extends Mock implements AccountStorage {}

class _FakeAccount extends Fake implements Account {}

class _BeforeLogOutCallbackMock extends Mock {
Future<void> call(Account account);
}

typedef _AccountStream = ({BuiltList<Account> accounts, Account? active});

void main() {
Expand All @@ -54,6 +60,7 @@ void main() {
setUpAll(() {
registerFallbackValue(_FakeUri());
registerFallbackValue(_FakePollRequest());
registerFallbackValue(_FakeAccount());
MockNeonStorage();
});

Expand Down Expand Up @@ -506,6 +513,20 @@ void main() {
when(() => appPassword.deleteAppPassword()).thenAnswer(
(_) async => _DynamiteResponseMock<core.AppPasswordDeleteAppPasswordResponseApplicationJson, void>(),
);

final callback1 = _BeforeLogOutCallbackMock();
when(() => callback1.call(any())).thenAnswer((_) async {});
repository.registerBeforeLogOutCallback(callback1.call);

final callback2 = _BeforeLogOutCallbackMock();
repository
..registerBeforeLogOutCallback(callback2.call)
..unregisterBeforeLogOutCallback(callback2.call);

final callback3 = _BeforeLogOutCallbackMock();
when(() => callback3.call(any())).thenAnswer((_) async {});
repository.registerBeforeLogOutCallback(callback3.call);

await repository.logOut(credentialsList.first.id);

await expectLater(
Expand All @@ -517,6 +538,10 @@ void main() {
),
);

verify(() => callback1(accountsList.first)).called(1);
verifyNever(() => callback2(accountsList.first));
verify(() => callback3(accountsList.first)).called(1);

verify(() => appPassword.deleteAppPassword()).called(1);
verify(() => storage.saveLastAccount(credentialsList[1].id)).called(1);
verify(() => storage.saveCredentials(any(that: equals([credentialsList[1]])))).called(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ class NotificationsPushRepository {
required OnMessageCallback onMessage,
}) : _accountRepository = accountRepository,
_storage = storage,
_onMessage = onMessage;
_onMessage = onMessage {
_accountRepository.registerBeforeLogOutCallback(_beforeLogOutCallback);
}

final AccountRepository _accountRepository;
final NotificationsPushStorage _storage;
Expand All @@ -48,6 +50,28 @@ class NotificationsPushRepository {
/// Closes all open resources of the repository.
void close() {
unawaited(_accountsListener?.cancel());
_accountRepository.unregisterBeforeLogOutCallback(_beforeLogOutCallback);
}

Future<void> _beforeLogOutCallback(Account account) async {
final subscriptions = await _storage.readSubscriptions();
var subscription = subscriptions[account.id];
if (subscription == null) {
return;
}

final pushDevice = subscription.pushDevice;
if (pushDevice != null) {
await _unregisterNextcloud(account.id, account, pushDevice);
subscription = subscription.rebuild((b) => b.pushDevice = null);
}

if (subscription.endpoint != null) {
await _unregisterUnifiedPush(account.id);
subscription = subscription.rebuild((b) => b.endpoint = null);
}

await _storage.updateSubscription(account.id, subscription);
}

/// Changes the used distributor to the new [distributor].
Expand Down Expand Up @@ -146,21 +170,21 @@ class NotificationsPushRepository {
if (_selectedDistributor == null) {
_log.fine('Push notifications disabled, removing all subscriptions');

await _unregisterUnifiedPush();
await _unregisterAllUnifiedPush();
return;
}

if (distributorChanged) {
_log.finer('UnifiedPush distributor changed to $_selectedDistributor');

await _unregisterUnifiedPush();
await _unregisterAllUnifiedPush();
await UnifiedPush.saveDistributor(_selectedDistributor!);
}

await _registerUnifiedPush();
await _registerAllUnifiedPush();
}

Future<void> _registerUnifiedPush() async {
Future<void> _registerAllUnifiedPush() async {
// Notifications will only work on accounts with app password
final accounts = (await _accountRepository.accounts.first).accounts;
for (final account in accounts.where((a) => a.credentials.appPassword != null)) {
Expand All @@ -170,7 +194,7 @@ class NotificationsPushRepository {
}
}

Future<void> _unregisterUnifiedPush() async {
Future<void> _unregisterAllUnifiedPush() async {
final subscriptions = await _storage.readSubscriptions();
for (final entry in subscriptions.entries) {
final accountID = entry.key;
Expand All @@ -184,10 +208,7 @@ class NotificationsPushRepository {
}

if (subscription.endpoint != null) {
_log.finer('Unregistering $accountID from UnifiedPush');

await UnifiedPush.unregister(accountID);

await _unregisterUnifiedPush(accountID);
subscription = subscription.rebuild((b) => b.endpoint = null);
}

Expand Down Expand Up @@ -229,4 +250,10 @@ class NotificationsPushRepository {
_log.warning('Failed to unregister $accountID at Nextcloud', error);
}
}

Future<void> _unregisterUnifiedPush(String accountID) async {
_log.finer('Unregistering $accountID from UnifiedPush');

await UnifiedPush.unregister(accountID);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ void main() {
void Function(String, String)? unifiedPushOnNewEndpoint;
void Function(String)? unifiedPushOnUnregistered;
void Function(Uint8List, String)? unifiedPushOnMessage;
late List<BeforeLogOutCallback> beforeLogOutCallbacks;

setUpAll(() {
registerFallbackValue(RSAPrivateKey(BigInt.zero, BigInt.zero, BigInt.zero, BigInt.zero));
Expand All @@ -72,6 +73,7 @@ void main() {
});

setUp(() {
beforeLogOutCallbacks = [];
accountsSubject = BehaviorSubject();
accountRepository = _AccountRepositoryMock();
when(() => accountRepository.accounts).thenAnswer((_) => accountsSubject.map((e) => (active: null, accounts: e)));
Expand All @@ -81,6 +83,12 @@ void main() {
return accountsSubject.value.singleWhereOrNull((account) => account.id == accountID);
},
);
when(() => accountRepository.registerBeforeLogOutCallback(any())).thenAnswer((invocation) {
beforeLogOutCallbacks.add(invocation.positionalArguments.first as BeforeLogOutCallback);
});
when(() => accountRepository.unregisterBeforeLogOutCallback(any())).thenAnswer((invocation) {
beforeLogOutCallbacks.remove(invocation.positionalArguments.first as BeforeLogOutCallback);
});

storage = _StorageMock();

Expand All @@ -104,6 +112,7 @@ void main() {

tearDown(() {
repository.close();
expect(beforeLogOutCallbacks, isEmpty);
unawaited(accountsSubject.close());
});

Expand Down Expand Up @@ -1728,6 +1737,122 @@ void main() {
});
});
});

group('Removes subscription when account is deleted', () {
test('Success at Nextcloud', () async {
when(
() => httpRequest(
'DELETE',
Uri.parse('https://cloud.example.com:8443/nextcloud/ocs/v2.php/apps/notifications/api/v2/push'),
any(),
any(),
),
).thenAnswer(
(_) => http.StreamedResponse(
Stream.value(
utf8.encode(
json.encode(
{
'ocs': {
'meta': {
'status': '',
'statuscode': 0,
},
'data': <dynamic, dynamic>{},
},
},
),
),
),
200,
headers: {
'content-type': 'application/json; charset=utf-8',
},
),
);

repository = NotificationsPushRepository(
accountRepository: accountRepository,
storage: storage,
onMessage: onMessageCallback,
);
await repository.initialize();

verify(() => unifiedPushPlatform.registerApp(account.id, [])).called(1);
verifyNever(() => unifiedPushPlatform.registerApp(any(), any()));

for (final callback in beforeLogOutCallbacks) {
await callback(account);
}

verify(() => unifiedPushPlatform.unregister(account.id)).called(1);
verifyNever(() => unifiedPushPlatform.unregister(any()));
verify(
() => httpRequest(
'DELETE',
Uri.parse('https://cloud.example.com:8443/nextcloud/ocs/v2.php/apps/notifications/api/v2/push'),
BuiltMap(
{
'Accept': 'application/json',
'Authorization': 'Bearer user1',
'OCS-APIRequest': 'true',
'user-agent': 'neon',
},
),
Uint8List(0),
),
).called(1);
verifyNever(() => httpRequest(any(), any(), any(), any()));
verify(() => storage.updateSubscription(account.id, PushSubscription())).called(1);
verifyNever(() => storage.updateSubscription(any(), any()));
});

test('Failure at Nextcloud', () async {
when(
() => httpRequest(
'DELETE',
Uri.parse('https://cloud.example.com:8443/nextcloud/ocs/v2.php/apps/notifications/api/v2/push'),
any(),
any(),
),
).thenAnswer((_) => http.StreamedResponse(const Stream.empty(), 500));

repository = NotificationsPushRepository(
accountRepository: accountRepository,
storage: storage,
onMessage: onMessageCallback,
);
await repository.initialize();

verify(() => unifiedPushPlatform.registerApp(account.id, [])).called(1);
verifyNever(() => unifiedPushPlatform.registerApp(any(), any()));

for (final callback in beforeLogOutCallbacks) {
await callback(account);
}

verify(() => unifiedPushPlatform.unregister(account.id)).called(1);
verifyNever(() => unifiedPushPlatform.unregister(any()));
verify(
() => httpRequest(
'DELETE',
Uri.parse('https://cloud.example.com:8443/nextcloud/ocs/v2.php/apps/notifications/api/v2/push'),
BuiltMap(
{
'Accept': 'application/json',
'Authorization': 'Bearer user1',
'OCS-APIRequest': 'true',
'user-agent': 'neon',
},
),
Uint8List(0),
),
).called(1);
verifyNever(() => httpRequest(any(), any(), any(), any()));
verify(() => storage.updateSubscription(account.id, PushSubscription())).called(1);
verifyNever(() => storage.updateSubscription(any(), any()));
});
});
});
});
});
Expand Down
Loading