Skip to content

Commit 44d9d55

Browse files
nickvergessennickvergessen
authored
Merge pull request #3101 from nextcloud/backport/3079/stable32
[stable32] chore: extend testing of `encryptAndSign` and make the methods discoverable by psalm
2 parents 4d48c7d + 4925fa8 commit 44d9d55

3 files changed

Lines changed: 89 additions & 26 deletions

File tree

‎lib/Push.php‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ public function pushToDevice(int $id, INotification $notification): void {
296296
}
297297

298298
try {
299-
$payload = json_encode($this->encryptAndSign($userKey, $device, $id, $notification, $isTalkNotification), JSON_THROW_ON_ERROR);
299+
$payload = json_encode($this->encryptAndSign($userKey->getPrivate(), $device, $id, $notification, $isTalkNotification), JSON_THROW_ON_ERROR);
300300

301301
$proxyServer = rtrim($device['proxyserver'], '/');
302302
if (!isset($this->payloadsToSend[$proxyServer])) {
@@ -393,7 +393,7 @@ public function pushDeleteToDevice(string $userId, ?array $notificationIds, stri
393393
}
394394

395395
if ($deleteAll) {
396-
$data = $this->encryptAndSignDelete($userKey, $device, null);
396+
$data = $this->encryptAndSignDelete($userKey->getPrivate(), $device, null);
397397
try {
398398
$this->payloadsToSend[$proxyServer][] = json_encode($data['payload'], JSON_THROW_ON_ERROR);
399399
} catch (\JsonException $e) {
@@ -403,7 +403,7 @@ public function pushDeleteToDevice(string $userId, ?array $notificationIds, stri
403403
$temp = $notificationIds;
404404

405405
while (!empty($temp)) {
406-
$data = $this->encryptAndSignDelete($userKey, $device, $temp);
406+
$data = $this->encryptAndSignDelete($userKey->getPrivate(), $device, $temp);
407407
$temp = $data['remaining'];
408408
try {
409409
$this->payloadsToSend[$proxyServer][] = json_encode($data['payload'], JSON_THROW_ON_ERROR);
@@ -602,7 +602,7 @@ protected function callSafelyForToken(IToken $token, string $method): ?int {
602602
}
603603

604604
/**
605-
* @param Key $userKey
605+
* @param string $userPrivateKey
606606
* @param array $device
607607
* @param int $id
608608
* @param INotification $notification
@@ -612,7 +612,7 @@ protected function callSafelyForToken(IToken $token, string $method): ?int {
612612
* @throws InvalidTokenException
613613
* @throws \InvalidArgumentException
614614
*/
615-
protected function encryptAndSign(Key $userKey, array $device, int $id, INotification $notification, bool $isTalkNotification): array {
615+
protected function encryptAndSign(string $userPrivateKey, array $device, int $id, INotification $notification, bool $isTalkNotification): array {
616616
$data = [
617617
'nid' => $id,
618618
'app' => $notification->getApp(),
@@ -621,9 +621,11 @@ protected function encryptAndSign(Key $userKey, array $device, int $id, INotific
621621
'id' => $notification->getObjectId(),
622622
];
623623

624+
$jsonData = (string)json_encode($data);
625+
624626
// Max length of encryption is ~240, so we need to make sure the subject is shorter.
625627
// Also, subtract two for encapsulating quotes will be added.
626-
$maxDataLength = 200 - strlen(json_encode($data)) - 2;
628+
$maxDataLength = 200 - strlen($jsonData) - 2;
627629
$data['subject'] = Util::shortenMultibyteString($notification->getParsedSubject(), $maxDataLength);
628630
if ($notification->getParsedSubject() !== $data['subject']) {
629631
$data['subject'] .= '…';
@@ -641,17 +643,17 @@ protected function encryptAndSign(Key $userKey, array $device, int $id, INotific
641643
}
642644

643645
$this->printInfo('Device public key size: ' . strlen($device['devicepublickey']));
644-
$this->printInfo('Data to encrypt is: ' . json_encode($data));
646+
$this->printInfo('Data to encrypt is: ' . $jsonData);
645647

646648
$padding = $this->appConfig->getAppValueString('push_encryption_padding', 'PKCS1') === 'OAEP' ? OPENSSL_PKCS1_OAEP_PADDING : OPENSSL_PKCS1_PADDING;
647-
if (!openssl_public_encrypt(json_encode($data), $encryptedSubject, $device['devicepublickey'], $padding)) {
648-
$error = openssl_error_string();
649+
if (!openssl_public_encrypt($jsonData, $encryptedSubject, $device['devicepublickey'], $padding)) {
650+
$error = openssl_error_string() ?: 'Unknown OpenSSL error';
649651
$this->log->error($error, ['app' => 'notifications']);
650652
$this->printInfo('<error>Error while encrypting data: "' . $error . '"</error>');
651653
throw new \InvalidArgumentException('Failed to encrypt message for device');
652654
}
653655

654-
if (openssl_sign($encryptedSubject, $signature, $userKey->getPrivate(), OPENSSL_ALGO_SHA512)) {
656+
if (openssl_sign($encryptedSubject, $signature, $userPrivateKey, OPENSSL_ALGO_SHA512)) {
655657
$this->printInfo('Signed encrypted push subject');
656658
} else {
657659
$this->printInfo('<error>Failed to signed encrypted push subject</error>');
@@ -670,15 +672,15 @@ protected function encryptAndSign(Key $userKey, array $device, int $id, INotific
670672
}
671673

672674
/**
673-
* @param Key $userKey
675+
* @param string $userPrivateKey
674676
* @param array $device
675677
* @param ?int[] $ids
676678
* @return array
677-
* @psalm-return array{remaining: list<int>, payload: array{deviceIdentifier: string, pushTokenHash: string, subject: string, signature: string, priority: string, type: string}}
679+
* @psalm-return array{remaining: array<array-key, int>, payload: array{deviceIdentifier: string, pushTokenHash: string, subject: string, signature: string, priority: string, type: string}}
678680
* @throws InvalidTokenException
679681
* @throws \InvalidArgumentException
680682
*/
681-
protected function encryptAndSignDelete(Key $userKey, array $device, ?array $ids): array {
683+
protected function encryptAndSignDelete(string $userPrivateKey, array $device, ?array $ids): array {
682684
$remainingIds = [];
683685
if ($ids === null) {
684686
$data = [
@@ -698,12 +700,13 @@ protected function encryptAndSignDelete(Key $userKey, array $device, ?array $ids
698700
}
699701

700702
$padding = $this->appConfig->getAppValueString('push_encryption_padding', 'PKCS1') === 'OAEP' ? OPENSSL_PKCS1_OAEP_PADDING : OPENSSL_PKCS1_PADDING;
701-
if (!openssl_public_encrypt(json_encode($data), $encryptedSubject, $device['devicepublickey'], $padding)) {
702-
$this->log->error(openssl_error_string(), ['app' => 'notifications']);
703+
if (!openssl_public_encrypt(json_encode($data, JSON_THROW_ON_ERROR), $encryptedSubject, $device['devicepublickey'], $padding)) {
704+
$error = openssl_error_string() ?: 'Unknown OpenSSL error';
705+
$this->log->error($error, ['app' => 'notifications']);
703706
throw new \InvalidArgumentException('Failed to encrypt message for device');
704707
}
705708

706-
openssl_sign($encryptedSubject, $signature, $userKey->getPrivate(), OPENSSL_ALGO_SHA512);
709+
openssl_sign($encryptedSubject, $signature, $userPrivateKey, OPENSSL_ALGO_SHA512);
707710
$base64EncryptedSubject = base64_encode($encryptedSubject);
708711
$base64Signature = base64_encode($signature);
709712

‎tests/Unit/PushTest.php‎

Lines changed: 70 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -434,16 +434,18 @@ public function testPushToDeviceNoFairUse(): void {
434434

435435
public static function dataPushToDeviceSending(): array {
436436
return [
437-
[true],
438-
[false],
437+
[true, 'PKCS1'],
438+
[true, 'OAEP'],
439+
[false, 'PKCS1'],
440+
[false, 'OAEP'],
439441
];
440442
}
441443

442444
/**
443445
* @dataProvider dataPushToDeviceSending
444446
*/
445-
public function testPushToDeviceSending(bool $isDebug): void {
446-
$push = $this->getPush(['createFakeUserObject', 'getDevicesForUser', 'encryptAndSign', 'deletePushToken', 'validateToken', 'deletePushTokenByDeviceIdentifier']);
447+
public function testPushToDeviceSending(bool $isDebug, string $padding): void {
448+
$push = $this->getPush(['createFakeUserObject', 'getDevicesForUser', 'deletePushToken', 'validateToken', 'deletePushTokenByDeviceIdentifier']);
447449

448450
/** @var INotification&MockObject $notification */
449451
$notification = $this->createMock(INotification::class);
@@ -459,38 +461,66 @@ public function testPushToDeviceSending(bool $isDebug): void {
459461
->with('valid')
460462
->willReturn($user);
461463

464+
$devicePublicKey = '-----BEGIN PUBLIC KEY-----
465+
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1SN9sDJGNifnEv/y1UkP
466+
tggJA0xks5b0WN/Ida3GYK4Zy/ZWTa3wAknCerKkZC2rrFbcP55HA/oSp8fuUJC3
467+
q4b59znuhGoQtvvdAwUx6qSIPheAGs/gfMpNWO/bfH02oBu+98eTkxciuNKPxBFk
468+
wRdSSUxsHwkzCOw+er6oxriVSkc7tsNVaXg+ZpzW15cUQugjT6JDDjg5ftSeGsLj
469+
VV70QXge4uD3ege/lsa1N8iUVCjeMJHobyQm/hhGE990b6BzTgOIC1pGsOPbOsZB
470+
/5n54G4EUX9dixSSF90fDJs83GWQ+AIjf/uHmj3vFMe1bnqIwq9P17+IWe5x9Z04
471+
FQIDAQAB
472+
-----END PUBLIC KEY-----';
473+
462474
$push->expects($this->once())
463475
->method('getDevicesForUser')
464476
->willReturn([
465477
[
466478
'proxyserver' => 'proxyserver1',
467479
'token' => 16,
468480
'apptype' => 'other',
481+
'devicepublickey' => $devicePublicKey,
482+
'deviceidentifier' => 'ident16',
483+
'pushtokenhash' => 'hash16',
469484
],
470485
[
471486
'proxyserver' => 'proxyserver1/',
472487
'token' => 23,
473488
'apptype' => 'other',
489+
'devicepublickey' => $devicePublicKey,
490+
'deviceidentifier' => 'ident23',
491+
'pushtokenhash' => 'hash23',
474492
],
475493
[
476494
'proxyserver' => 'badrequest',
477495
'token' => 42,
478496
'apptype' => 'other',
497+
'devicepublickey' => $devicePublicKey,
498+
'deviceidentifier' => 'ident42',
499+
'pushtokenhash' => 'hash42',
479500
],
480501
[
481502
'proxyserver' => 'unavailable',
482503
'token' => 48,
483504
'apptype' => 'other',
505+
'devicepublickey' => $devicePublicKey,
506+
'deviceidentifier' => 'ident48',
507+
'pushtokenhash' => 'hash48',
484508
],
485509
[
486510
'proxyserver' => 'ok',
487511
'token' => 64,
488512
'apptype' => 'other',
513+
'devicepublickey' => $devicePublicKey,
514+
'deviceidentifier' => 'ident64',
515+
'pushtokenhash' => 'hash64',
489516
],
490517
[
491518
'proxyserver' => 'badrequest-with-devices',
492519
'token' => 128,
493520
'apptype' => 'other',
521+
'devicepublickey' => $devicePublicKey,
522+
'deviceidentifier' => 'ident128',
523+
'pushtokenhash' => 'hash128',
494524
],
495525
]);
496526

@@ -506,6 +536,12 @@ public function testPushToDeviceSending(bool $isDebug): void {
506536
['support', 'subscription_key', '', ''],
507537
]);
508538

539+
$this->appConfig
540+
->expects($this->exactly(6))
541+
->method('getAppValueString')
542+
->with('push_encryption_padding', 'PKCS1')
543+
->willReturn($padding);
544+
509545
$this->l10nFactory
510546
->method('getUserLanguage')
511547
->with($user)
@@ -519,6 +555,36 @@ public function testPushToDeviceSending(bool $isDebug): void {
519555
/** @var Key&MockObject $key */
520556
$key = $this->createMock(Key::class);
521557

558+
$key->method('getPrivate')
559+
->willReturn('-----BEGIN PRIVATE KEY-----
560+
MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCkGi7rj6BWi9U8
561+
lptb1rBKVFENFJjq690Ap8wAdR9cxGYE+nzdFxlcRxh17GwyWaBDEPWtL5WDPxi0
562+
8XjQWpwfYFzjMLLzGMZaj7Jiam5rJM12qFEvYyw9s26LzebeLTzvp28L2TIWCswi
563+
dM0OvrYGGvL9f+N+6Oev5cwiSJk0WrHceL+P/wFH3mmOdOBHmKkofeDZoS1XkNOE
564+
o1EpwepvDibUjc3vHPk1IHg4hsfKk1O9+uHiBf9xqm//M9MKrM9mlOButbo2bOhu
565+
1YN/2wKww7bU/c7JYgVeljpfN0sMuKmhMiuje2KqOU8v8yIREkKMyxd4+B+s3eNG
566+
PdVBf57FAgMBAAECggEAAMegdOm3xPyXMk+f0yNlh5kzDKE1itCL4aYIA6LZnLmp
567+
bHHJWZI4kTT1IHPdgDeYlPBjgAOuEFEsjhLySoXwoF4BIIgErCKD9xa9pBIojmVm
568+
LovlNfFlvScDKgsjUed+5ZguDLcYO7gyQvqTzS6ivXzq6TezeVDfeUgBTg3JG6n/
569+
BjgIek1jV4q9OrQTOdR3eAPwW6pbLG820OB87Cx/A0JHdEgCevFCHmMUbk7yXQrh
570+
W/JwTrnV66LBPjSWQcsHUNWrl2nqV08BYoC1uy32uTpUlejCdWDuHuB1DYOpn0KY
571+
nyiVTNF/MZ0E9sOsKKgbodBz4n3cc6v7NckpFpKMHwKBgQDUeaakKYwatNg6YEHP
572+
OB3iH62bAe7Ej6vg0EPYgr6fzRjAETzTrpZZfzmrGwTxFBczFrx8hOPsyDkpiEU5
573+
77L90jMOb9NSasjS8Pv0GM2LY/8H8Ck4RwXF/E3maSbc2bAY9ZXH6m82YW6iHnfc
574+
G9L0xVCq549axd1tWUnXdukiuwKBgQDFt9KDtlwMutuQOj+Eup7L0sMAxO4jlghH
575+
hTxSAqQ5mlodbQhULXRk4QWGMjjKG0y4XY2rw+VgBQzFT8W3jlThR3BMWKNv3P+M
576+
zlzkoxcoOio7K25im4Yb0NheOnaxaqhLcRwXcxa5E9kn+108xYIYAACZgmkZo7ab
577+
PCoQicnsfwKBgAzyEIYmBeRGqnn8DWZru95gIbq1BnAxdL5w0gFqDeU8oMprAnK/
578+
S2fOiZv0PHvXxoYVV4yaqCxwEpOGOvmJsjUmzneNtqlp2iyIBEHeFP/uKsa4Cjrk
579+
kOR8N97W/0grd0A+Dk8s6HO+wffctV7SzyqcrwqKq0BTl+cmrooTM6crAoGATjjT
580+
iFh1QnQKuZzR1GkgufLAQ2Wl8V5CGEmV+7wfzMpMLKgeS29QRTjhPp5P6WWzjJ02
581+
l2YBMWPOEaHlzyD4Y8gnnYzT3EXKtKJQDgSX/MpGOvKL0WdGP2r4rw7iNn7D5lTx
582+
kDVwH/jCSRchZBGfzm7xzcnSWtpyPCgpXDGnOXECgYEAyqDrBeKlUDDltoe6xs4K
583+
gEG+V5E5cwZqvSoOlYHqbtP9Dms6z6G8TvPZblwlbgwXFofrd5LN/vK3pZPiU5Y+
584+
sd7MhWnjKf7EX9GJD0VhLabFY/KrloJkyL7gOY21xFvmnNqwvH60eOxbVPzlYjaN
585+
96rK6qkqEdUgXj0CpJZHAMw=
586+
-----END PRIVATE KEY-----');
587+
522588
$this->keyManager->expects($this->once())
523589
->method('getKey')
524590
->with($user)
@@ -528,10 +594,6 @@ public function testPushToDeviceSending(bool $isDebug): void {
528594
->method('validateToken')
529595
->willReturn(true);
530596

531-
$push->expects($this->exactly(6))
532-
->method('encryptAndSign')
533-
->willReturn(['Payload']);
534-
535597
$push->expects($this->never())
536598
->method('deletePushToken');
537599

‎tests/psalm-baseline.xml‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,6 @@
2727
<code><![CDATA[$this->keyManager]]></code>
2828
<code><![CDATA[$this->tokenProvider]]></code>
2929
<code><![CDATA[ClientException]]></code>
30-
<code><![CDATA[Key]]></code>
31-
<code><![CDATA[Key]]></code>
3230
<code><![CDATA[ServerException]]></code>
3331
<code><![CDATA[protected]]></code>
3432
<code><![CDATA[protected]]></code>

0 commit comments

Comments
 (0)