Skip to content

Commit 44aa123

Browse files
authored
feat(mail): highlight warnings in account security emails
Put the key sentence of the remote wipe, password changed, email changed and code integrity emails into a warning or info note. The email changed mail also shows the previous and new address in a details card. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com>
1 parent 470b729 commit 44aa123

6 files changed

Lines changed: 211 additions & 11 deletions

File tree

‎apps/settings/lib/Hooks.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
use OCP\IUserManager;
2222
use OCP\IUserSession;
2323
use OCP\L10N\IFactory;
24+
use OCP\Mail\EMailDetails;
25+
use OCP\Mail\IEMailTemplate;
2426
use OCP\Mail\IMailer;
2527
use OCP\User\Events\PasswordUpdatedEvent;
2628
use OCP\User\Events\UserChangedEvent;
@@ -109,7 +111,8 @@ public function onChangePassword(PasswordUpdatedEvent $handle): void {
109111
$template->setSubject($l->t('Password for %1$s changed on %2$s', [$user->getDisplayName(), $instanceName]));
110112
$template->addHeader();
111113
$template->addHeading($l->t('Password changed for %s', [$user->getDisplayName()]), false);
112-
$template->addBodyText($text . ' ' . $l->t('If you did not request this, please contact an administrator.'));
114+
$template->addBodyText($text);
115+
$template->addBodyNote($l->t('If you did not request this, please contact an administrator.'), '', IEMailTemplate::NOTE_WARNING);
113116
$template->addFooter();
114117

115118
$message = $this->mailer->createMessage();
@@ -178,10 +181,17 @@ public function onChangeEmail(UserChangedEvent $handle): void {
178181
$template->setSubject($l->t('Email address for %1$s changed on %2$s', [$user->getDisplayName(), $instanceName]));
179182
$template->addHeader();
180183
$template->addHeading($l->t('Email address changed for %s', [$user->getDisplayName()]), false);
181-
$template->addBodyText($text . ' ' . $l->t('If you did not request this, please contact an administrator.'));
184+
$template->addBodyText($text);
185+
$details = (new EMailDetails($user->getDisplayName()))->setInitials($user->getDisplayName());
186+
$details->addRow($l->t('Previous email address'))->text($oldMailAddress);
187+
$newMailAddress = $details->addRow($l->t('New email address'));
182188
if ($user->getEMailAddress()) {
183-
$template->addBodyText($l->t('The new email address is %s', [$user->getEMailAddress()]));
189+
$newMailAddress->text($user->getEMailAddress());
190+
} else {
191+
$newMailAddress->muted($l->t('No email address set'));
184192
}
193+
$template->addBodyDetails($details);
194+
$template->addBodyNote($l->t('If you did not request this, please contact an administrator.'), '', IEMailTemplate::NOTE_WARNING);
185195
$template->addFooter();
186196

187197
$message = $this->mailer->createMessage();

‎apps/settings/tests/HooksTest.php‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Settings\Tests;
11+
12+
use OCA\Settings\Hooks;
13+
use OCP\Activity\IEvent;
14+
use OCP\Activity\IManager as IActivityManager;
15+
use OCP\Defaults;
16+
use OCP\IConfig;
17+
use OCP\IGroupManager;
18+
use OCP\IL10N;
19+
use OCP\IURLGenerator;
20+
use OCP\IUser;
21+
use OCP\IUserManager;
22+
use OCP\IUserSession;
23+
use OCP\L10N\IFactory;
24+
use OCP\Mail\EMailDetails;
25+
use OCP\Mail\EMailDetailsRow;
26+
use OCP\Mail\IEMailTemplate;
27+
use OCP\Mail\IMailer;
28+
use OCP\Mail\IMessage;
29+
use OCP\User\Events\PasswordUpdatedEvent;
30+
use OCP\User\Events\UserChangedEvent;
31+
use PHPUnit\Framework\MockObject\MockObject;
32+
use Test\TestCase;
33+
34+
class HooksTest extends TestCase {
35+
private IMailer&MockObject $mailer;
36+
private IEMailTemplate&MockObject $template;
37+
private IUser&MockObject $user;
38+
private Hooks $hooks;
39+
40+
protected function setUp(): void {
41+
parent::setUp();
42+
43+
$activityManager = $this->createMock(IActivityManager::class);
44+
$activityManager->method('generateEvent')->willReturn($this->createMock(IEvent::class));
45+
46+
$urlGenerator = $this->createMock(IURLGenerator::class);
47+
$urlGenerator->method('getAbsoluteURL')->willReturn('https://cloud.example.com/');
48+
49+
$l10n = $this->createMock(IL10N::class);
50+
$l10n->method('t')->willReturnCallback(fn (string $text, array $parameters = []): string => vsprintf($text, $parameters));
51+
$l10nFactory = $this->createMock(IFactory::class);
52+
$l10nFactory->method('get')->with('settings')->willReturn($l10n);
53+
54+
$this->user = $this->createMock(IUser::class);
55+
$this->user->method('getUID')->willReturn('alice');
56+
$this->user->method('getDisplayName')->willReturn('Alice');
57+
$this->user->method('getLastLogin')->willReturn(1);
58+
59+
$userSession = $this->createMock(IUserSession::class);
60+
$userSession->method('getUser')->willReturn($this->user);
61+
62+
$this->template = $this->createMock(IEMailTemplate::class);
63+
$this->mailer = $this->createMock(IMailer::class);
64+
$this->mailer->method('createEMailTemplate')->willReturn($this->template);
65+
$this->mailer->method('createMessage')->willReturn($this->createMock(IMessage::class));
66+
67+
$this->hooks = new Hooks(
68+
$activityManager,
69+
$this->createMock(IGroupManager::class),
70+
$this->createMock(IUserManager::class),
71+
$userSession,
72+
$urlGenerator,
73+
$this->mailer,
74+
$this->createMock(IConfig::class),
75+
$l10nFactory,
76+
$this->createMock(Defaults::class),
77+
);
78+
}
79+
80+
public function testPasswordChangedMail(): void {
81+
$this->user->method('getEMailAddress')->willReturn('alice@example.com');
82+
83+
$this->template->expects($this->once())->method('addBodyText')
84+
->with('Your password on https://cloud.example.com/ was changed.');
85+
$this->template->expects($this->once())->method('addBodyNote')
86+
->with('If you did not request this, please contact an administrator.', '', IEMailTemplate::NOTE_WARNING);
87+
$this->mailer->expects($this->once())->method('send');
88+
89+
$this->hooks->handle(new PasswordUpdatedEvent($this->user, 'secret'));
90+
}
91+
92+
public function testEmailChangedMail(): void {
93+
$this->user->method('getEMailAddress')->willReturn('new@example.com');
94+
95+
$this->template->expects($this->once())->method('addBodyText')
96+
->with('Your email address on https://cloud.example.com/ was changed.');
97+
$this->template->expects($this->once())->method('addBodyDetails')
98+
->with($this->callback(fn (EMailDetails $details): bool => $this->detailsToArray($details) === [
99+
'title' => 'Alice',
100+
'initials' => 'Alice',
101+
'rows' => [
102+
['Previous email address', [[EMailDetailsRow::PART_TEXT, 'old@example.com']]],
103+
['New email address', [[EMailDetailsRow::PART_TEXT, 'new@example.com']]],
104+
],
105+
]));
106+
$this->template->expects($this->once())->method('addBodyNote')
107+
->with('If you did not request this, please contact an administrator.', '', IEMailTemplate::NOTE_WARNING);
108+
$this->mailer->expects($this->once())->method('send');
109+
110+
$this->hooks->handle(new UserChangedEvent($this->user, 'eMailAddress', 'new@example.com', 'old@example.com'));
111+
}
112+
113+
public function testEmailRemovedMail(): void {
114+
$this->user->method('getEMailAddress')->willReturn(null);
115+
116+
$this->template->expects($this->once())->method('addBodyDetails')
117+
->with($this->callback(fn (EMailDetails $details): bool => $this->detailsToArray($details)['rows'] === [
118+
['Previous email address', [[EMailDetailsRow::PART_TEXT, 'old@example.com']]],
119+
['New email address', [[EMailDetailsRow::PART_MUTED, 'No email address set']]],
120+
]));
121+
$this->mailer->expects($this->once())->method('send');
122+
123+
$this->hooks->handle(new UserChangedEvent($this->user, 'eMailAddress', null, 'old@example.com'));
124+
}
125+
126+
private function detailsToArray(EMailDetails $details): array {
127+
return [
128+
'title' => $details->getTitle(),
129+
'initials' => $details->getInitialsName(),
130+
'rows' => array_map(
131+
fn (EMailDetailsRow $row): array => [
132+
$row->getLabel(),
133+
array_map(fn (array $part): array => [$part['type'], $part['text']], $row->getParts()),
134+
],
135+
$details->getRows(),
136+
),
137+
];
138+
}
139+
}

‎core/BackgroundJobs/CheckCodeIntegrityJob.php‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use OCP\IURLGenerator;
2222
use OCP\IUser;
2323
use OCP\L10N\IFactory;
24+
use OCP\Mail\IEMailTemplate;
2425
use OCP\Mail\IMailer;
2526
use OCP\Notification\IManager as INotificationManager;
2627
use Psr\Log\LoggerInterface;
@@ -196,9 +197,7 @@ private function sendMail(IUser $admin, array $summary): void {
196197
$template->setSubject($l->t('The code integrity check result has changed'));
197198
$template->addHeader();
198199
$template->addHeading($l->t('The code integrity check result has changed'));
199-
foreach (self::formatSummary($l, $summary) as $sentence) {
200-
$template->addBodyText($sentence);
201-
}
200+
$template->addBodyNote(implode("\n", self::formatSummary($l, $summary)), '', IEMailTemplate::NOTE_WARNING);
202201
$template->addBodyButton(
203202
$l->t('Review integrity check results'),
204203
$this->urlGenerator->linkToRouteAbsolute('settings.AdminSettings.index', ['section' => 'overview']),

‎lib/private/Authentication/Listeners/RemoteWipeEmailListener.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\IUser;
2020
use OCP\IUserManager;
2121
use OCP\L10N\IFactory as IL10nFactory;
22+
use OCP\Mail\IEMailTemplate;
2223
use OCP\Mail\IMailer;
2324
use OCP\Mail\IMessage;
2425
use Psr\Log\LoggerInterface;
@@ -107,8 +108,10 @@ private function getWipingStartedMessage(RemoteWipeStarted $event, IUser $user):
107108
$htmlHeading,
108109
$plainHeading
109110
);
110-
$emailTemplate->addBodyText(
111-
$this->l10n->t('Device or application »%s« has started the remote wipe process. You will receive another email once the process has finished', [$event->getToken()->getName()])
111+
$emailTemplate->addBodyNote(
112+
$this->l10n->t('Device or application »%s« has started the remote wipe process. You will receive another email once the process has finished', [$event->getToken()->getName()]),
113+
'',
114+
IEMailTemplate::NOTE_WARNING,
112115
);
113116
$emailTemplate->addFooter();
114117
$message->setTo([$user->getEMailAddress()]);
@@ -135,8 +138,10 @@ private function getWipingFinishedMessage(RemoteWipeFinished $event, IUser $user
135138
$htmlHeading,
136139
$plainHeading
137140
);
138-
$emailTemplate->addBodyText(
139-
$this->l10n->t('Device or application »%s« has finished the remote wipe process.', [$event->getToken()->getName()])
141+
$emailTemplate->addBodyNote(
142+
$this->l10n->t('Device or application »%s« has finished the remote wipe process.', [$event->getToken()->getName()]),
143+
'',
144+
IEMailTemplate::NOTE_INFO,
140145
);
141146
$emailTemplate->addFooter();
142147
$message->setTo([$user->getEMailAddress()]);

‎tests/Core/BackgroundJobs/CheckCodeIntegrityJobTest.php‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ class CheckCodeIntegrityJobTest extends TestCase {
4949
private IGroupManager&MockObject $groupManager;
5050
private INotificationManager&MockObject $notificationManager;
5151
private IMailer&MockObject $mailer;
52+
private IEMailTemplate&MockObject $template;
5253
private IL10N&MockObject $l10n;
5354
/** @var list<string> recipients collected by expectAdminNotifications() */
5455
private array $users = [];
@@ -76,7 +77,8 @@ protected function setUp(): void {
7677
=> vsprintf(str_replace('%n', (string)$count, $count === 1 ? $singular : $plural), $parameters)
7778
);
7879

79-
$this->mailer->method('createEMailTemplate')->willReturn($this->createMock(IEMailTemplate::class));
80+
$this->template = $this->createMock(IEMailTemplate::class);
81+
$this->mailer->method('createEMailTemplate')->willReturn($this->template);
8082
$this->mailer->method('createMessage')->willReturn($this->createMock(IMessage::class));
8183

8284
$this->notificationManager->method('createNotification')->willReturnCallback(
@@ -182,6 +184,13 @@ public function testNotifiesAndMailsAdminsWhenCheckStartsFailing(): void {
182184
$this->notificationManager->expects($this->never())->method('markProcessed');
183185
$this->expectAdminNotifications($fingerprint, ['files' => 1, 'unverified' => []]);
184186
$this->mailer->expects($this->once())->method('send');
187+
$this->template->expects($this->never())->method('addBodyText');
188+
$this->template->expects($this->once())->method('addBodyNote')->with(
189+
'1 file does not match the signed release. It may have been modified or added without authorization.',
190+
'',
191+
IEMailTemplate::NOTE_WARNING,
192+
);
193+
$this->template->expects($this->once())->method('addBodyButton');
185194
$this->expectStoredFingerprint($fingerprint);
186195

187196
self::invokePrivate($this->job, 'run', [null]);
@@ -218,6 +227,23 @@ public function testReplacesNotificationWhenFailureChanges(): void {
218227
self::invokePrivate($this->job, 'run', [null]);
219228
}
220229

230+
public function testMailPutsEverySentenceInOneWarningNote(): void {
231+
$this->mockAdmins();
232+
$results = self::FAILED_RESULT;
233+
$results['files'] = ['EXCEPTION' => ['class' => \Exception::class, 'message' => 'Signature data not found.']];
234+
$this->checker->method('getResults')->willReturn($results);
235+
$this->appConfig->method('getValueString')->willReturn('');
236+
237+
$this->template->expects($this->once())->method('addBodyNote')->with(
238+
"1 file does not match the signed release. It may have been modified or added without authorization.\n"
239+
. 'The signature of files is missing or invalid, so it could not be verified.',
240+
'',
241+
IEMailTemplate::NOTE_WARNING,
242+
);
243+
244+
self::invokePrivate($this->job, 'run', [null]);
245+
}
246+
221247
public function testClearsNotificationWhenCheckPassesAgain(): void {
222248
$this->checker->method('getResults')->willReturn([]);
223249
$previous = $this->fingerprint(self::FAILED_RESULT);

‎tests/lib/Authentication/Listeners/RemoteWipeEmailListenerTest.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCP\IUser;
2121
use OCP\IUserManager;
2222
use OCP\L10N\IFactory;
23+
use OCP\Mail\IEMailTemplate;
2324
use OCP\Mail\IMailer;
2425
use OCP\Mail\IMessage;
2526
use PHPUnit\Framework\MockObject\MockObject;
@@ -105,6 +106,7 @@ public function testHandleRemoteWipeStarted(): void {
105106
$token = $this->createMock(IToken::class);
106107
$event = new RemoteWipeStarted($token);
107108
$token->method('getUID')->willReturn('nope');
109+
$token->method('getName')->willReturn('Phone');
108110
$user = $this->createMock(IUser::class);
109111
$this->getAutoMock(IUserManager::class)->expects($this->once())
110112
->method('get')
@@ -115,6 +117,15 @@ public function testHandleRemoteWipeStarted(): void {
115117
$this->getAutoMock(IMailer::class)->expects($this->once())
116118
->method('createMessage')
117119
->willReturn($message);
120+
$template = $this->createMock(IEMailTemplate::class);
121+
$this->getAutoMock(IMailer::class)->expects($this->once())
122+
->method('createEMailTemplate')
123+
->with('auth.RemoteWipeStarted')
124+
->willReturn($template);
125+
$template->expects($this->never())->method('addBodyText');
126+
$template->expects($this->once())
127+
->method('addBodyNote')
128+
->with('Device or application »Phone« has started the remote wipe process. You will receive another email once the process has finished', '', IEMailTemplate::NOTE_WARNING);
118129
$message->expects($this->once())
119130
->method('setTo')
120131
->with($this->equalTo(['user@domain.org']));
@@ -180,6 +191,7 @@ public function testHandleRemoteWipeFinished(): void {
180191
$token = $this->createMock(IToken::class);
181192
$event = new RemoteWipeFinished($token);
182193
$token->method('getUID')->willReturn('nope');
194+
$token->method('getName')->willReturn('Phone');
183195
$user = $this->createMock(IUser::class);
184196
$this->getAutoMock(IUserManager::class)->expects($this->once())
185197
->method('get')
@@ -190,6 +202,15 @@ public function testHandleRemoteWipeFinished(): void {
190202
$this->getAutoMock(IMailer::class)->expects($this->once())
191203
->method('createMessage')
192204
->willReturn($message);
205+
$template = $this->createMock(IEMailTemplate::class);
206+
$this->getAutoMock(IMailer::class)->expects($this->once())
207+
->method('createEMailTemplate')
208+
->with('auth.RemoteWipeFinished')
209+
->willReturn($template);
210+
$template->expects($this->never())->method('addBodyText');
211+
$template->expects($this->once())
212+
->method('addBodyNote')
213+
->with('Device or application »Phone« has finished the remote wipe process.', '', IEMailTemplate::NOTE_INFO);
193214
$message->expects($this->once())
194215
->method('setTo')
195216
->with($this->equalTo(['user@domain.org']));

0 commit comments

Comments
 (0)