NEW email notification for MFA settings update #134
Conversation
NightJar
force-pushed
the
pulls/4/notifications
branch
from
b1dbd50 to
4016470
Compare
There was a problem hiding this comment.
Awesome. Haven't had a chance to check it out and have a play. I have one super minor semantics comment. Notification isn't really a Service any longer - because it's stateful. You could make it stateless by turning send into the only method that takes the contructor args. Usage within the code would then be:
Notification::singleton()->send($member, $template, $data);
You could then to the private static $dependencies configuration in the service class for the Email dependency.
NightJar
force-pushed
the
pulls/4/notifications
branch
2 times, most recently
from
9936645 to
11e6d91
Compare
|
Cool, I adapted to along the lines of your comment too @ScopeyNZ - however I didn't see the point in dependencies as the execution already fetches a named service. Leaving a member variable would still have state, whether it be given or installed via Injector. |
|
Broken build from race condition. Rerun has made them green. |
NightJar
force-pushed
the
pulls/4/notifications
branch
2 times, most recently
from
7b7bbef to
e27e005
Compare
NightJar
force-pushed
the
pulls/4/notifications
branch
3 times, most recently
from
20db451 to
99a1b91
Compare
|
addressed and tests passed. |
src/BackupCode/VerifyHandler.php
Outdated
| @@ -58,6 +59,14 @@ public function verify(HTTPRequest $request, StoreInterface $store, RegisteredMe | |||
| array_splice($candidates, $index, 1); | |||
| $registeredMethod->Data = json_encode($candidates); | |||
| $registeredMethod->write(); | |||
| Notification::singleton()->send( | |||
src/Service/Notification.php
Outdated
| */ | ||
| public function send(Member $member, string $template, array $data = []): bool | ||
| { | ||
| $this->email |
There was a problem hiding this comment.
This will make the email object a singleton (by using it in DI) - is that what you expected?
There was a problem hiding this comment.
err, no. thanks for pointing that out.
| * Ecapsulates setting up an Email in order to allow for Dependency Injection and to avoid introducing a hard | ||
| * coupling to the SilverStripe core Email class in code that consumes this class. | ||
| */ | ||
| class Notification |
There was a problem hiding this comment.
Did you want to rename the class? The reason I suggested it because $dependencies = ['NotificationService' => Notification::class] is not as verbose as NotificationService::class despite it being namespaced. If you think it's fine then I'm happy too. You could also use Notification\Service::class, or leave it as is. The property name helps 👍
As a precaution users should be notified whenever their account details pertaining to security are updated, in order to let them respond to what could be a security breach. The easiest manner to do this is to send an email to their registered address.
NightJar
force-pushed
the
pulls/4/notifications
branch
from
99a1b91 to
02fa370
Compare
| $this->extend('onAfterSend', $email, $sendResult); | ||
|
|
||
| // Could no longer be a bool as a result of the extension point above, so is cast to bool. | ||
| return (bool) $sendResult; |
There was a problem hiding this comment.
You could also use a SilverStripe\MFA\State\Result object which could provide more context. That was extensions can say why they're changing it to a failure as well. We could also provide $ex->getMessage() as context when catching exceptions and setting it to be a failure. I think a bool is fine for now but maybe something you might want to follow up on later 👍
As a precaution users should be notified whenever their account details pertaining to security are updated, in order to let them respond to what could be a security breach. The easiest manner to do this is to send an email to their registered address.
Closes #67