diff --git a/lib/Db/SuspiciousLoginMapper.php b/lib/Db/SuspiciousLoginMapper.php index f39e5dd1..f996fa6d 100644 --- a/lib/Db/SuspiciousLoginMapper.php +++ b/lib/Db/SuspiciousLoginMapper.php @@ -33,7 +33,7 @@ public function __construct(IDBConnection $db) { parent::__construct($db, 'suspicious_login'); } - public function findRecent(string $uid, string $ip, $start): array { + public function findRelated(string $uid, string $ip, SuspiciousLogin $login, int $start): array { $qb = $this->db->getQueryBuilder(); $query = $qb @@ -41,6 +41,19 @@ public function findRecent(string $uid, string $ip, $start): array { ->from($this->getTableName()) ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid))) ->andWhere($qb->expr()->eq('ip', $qb->createNamedParameter($ip))) + ->andWhere($qb->expr()->gte('created_at', $qb->createNamedParameter($start))) + ->andWhere($qb->expr()->neq('id', $qb->createNamedParameter($login->getId()))); + + return $this->findEntities($query); + } + + public function findRecentByUid(string $uid, int $start) { + $qb = $this->db->getQueryBuilder(); + + $query = $qb + ->select('*') + ->from($this->getTableName()) + ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid))) ->andWhere($qb->expr()->gte('created_at', $qb->createNamedParameter($start))); return $this->findEntities($query); diff --git a/lib/Service/LoginClassifier.php b/lib/Service/LoginClassifier.php index c5555778..d7e88dc8 100644 --- a/lib/Service/LoginClassifier.php +++ b/lib/Service/LoginClassifier.php @@ -121,15 +121,15 @@ public function process(string $uid, string $ip) { $this->logger->warning("Detected a login from a suspicious login. user=$uid ip=$ip strategy=" . $strategy::getTypeName()); - $this->persistSuspiciousLogin($uid, $ip); - $this->notifyUser($uid, $ip); + $login = $this->persistSuspiciousLogin($uid, $ip); + $this->notifyUser($uid, $ip, $login); } /** * @param string $uid * @param string $ip */ - private function persistSuspiciousLogin(string $uid, string $ip) { + private function persistSuspiciousLogin(string $uid, string $ip): SuspiciousLogin { try { $entity = new SuspiciousLogin(); $entity->setUid($uid); @@ -139,6 +139,8 @@ private function persistSuspiciousLogin(string $uid, string $ip) { $entity->setCreatedAt($this->timeFactory->getTime()); $this->mapper->insert($entity); + + return $entity; } catch (Throwable $ex) { $this->logger->critical("could not save the details of a suspicious login"); $this->logger->logException($ex); @@ -148,13 +150,29 @@ private function persistSuspiciousLogin(string $uid, string $ip) { /** * @param string $uid * @param string $ip + * @param SuspiciousLogin $login */ - private function notifyUser(string $uid, string $ip): void { + private function notifyUser(string $uid, string $ip, SuspiciousLogin $login): void { $now = $this->timeFactory->getTime(); - $twoHoursAgo = $now - 60*60*2; - // We just inserted one alert – are there more with these params? - if (count($this->mapper->findRecent($uid, $ip, $twoHoursAgo)) > 1) { - $this->logger->debug("Notification for $uid:$ip already sent"); + + // Assuming that a suspicious IP is most likely one that hasn't been seen before + // (for this user), we'll not send another notification until the data is used + // for the model training + $secondsSinceLastTraining = TrainingDataConfig::default()->getThreshold() * 60 * 60 * 24; + if (count($this->mapper->findRelated($uid, $ip, $login, $now - $secondsSinceLastTraining)) > 0) { + $this->logger->debug("Notification for $uid:$ip already sent, waiting until the next training period"); + return; + } + + $lastTwoDays = count($this->mapper->findRecentByUid($uid, $now - 60 * 60 * 24 * 2)); + if ($lastTwoDays > 10) { + $this->logger->warning("Suspicious login peak detected: $uid received $lastTwoDays alerts in the last two days"); + return; + } + + $lastHour = count($this->mapper->findRecentByUid($uid, $now - 60 * 60)); + if ($lastHour > 3) { + $this->logger->warning("Suspicious login peak detected: $uid received $lastTwoDays alerts in the last hour"); return; } diff --git a/tests/Unit/Service/LoginClassifierTest.php b/tests/Unit/Service/LoginClassifierTest.php new file mode 100644 index 00000000..466399d2 --- /dev/null +++ b/tests/Unit/Service/LoginClassifierTest.php @@ -0,0 +1,184 @@ + + * + * @author 2019 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OCA\SuspiciousLogin\Tests\Unit\Service; + +use ChristophWurst\Nextcloud\Testing\TestCase; +use OCA\SuspiciousLogin\Db\SuspiciousLogin; +use OCA\SuspiciousLogin\Db\SuspiciousLoginMapper; +use OCA\SuspiciousLogin\Service\EstimatorService; +use OCA\SuspiciousLogin\Service\Ipv4Strategy; +use OCA\SuspiciousLogin\Service\LoginClassifier; +use OCA\SuspiciousLogin\Service\TrainingDataConfig; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\ILogger; +use OCP\IRequest; +use PHPUnit\Framework\MockObject\MockObject; + +class LoginClassifierTest extends TestCase { + + /** @var EstimatorService|MockObject */ + private $estimatorService; + + /** @var IRequest|MockObject */ + private $request; + + /** @var SuspiciousLoginMapper|MockObject */ + private $mapper; + + /** @var ILogger|MockObject */ + private $logger; + + /** @var ITimeFactory|MockObject */ + private $timeFactory; + + /** @var IEventDispatcher|MockObject */ + private $dispatcher; + /** @var LoginClassifier */ + private $classifier; + + protected function setUp() { + parent::setUp(); + + $this->estimatorService = $this->createMock(EstimatorService::class); + $this->request = $this->createMock(IRequest::class); + $this->logger = $this->createMock(ILogger::class); + $this->mapper = $this->createMock(SuspiciousLoginMapper::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->dispatcher = $this->createMock(IEventDispatcher::class); + + $this->classifier = new LoginClassifier( + $this->estimatorService, + $this->request, + $this->logger, + $this->mapper, + $this->timeFactory, + $this->dispatcher + ); + } + + public function testProcessAlertAlreadySent(): void { + $this->timeFactory->method('getTime')->willReturn(1000000); + $this->estimatorService->expects($this->once()) + ->method('predict') + ->with('user', '1.2.3.4', $this->equalTo(new Ipv4Strategy())) + ->willReturn(false); + $this->mapper->expects($this->at(1)) + ->method('findRelated') + ->with( + 'user', + '1.2.3.4', + $this->anything(), + 1000000-60*60*24*TrainingDataConfig::default()->getThreshold() + ) + ->willReturn([ + new SuspiciousLogin(), + ]); + $this->dispatcher->expects($this->never()) + ->method('dispatch'); + + $this->classifier->process('user', '1.2.3.4'); + } + + public function testProcessTwoDayPeakReached(): void { + $this->timeFactory->method('getTime')->willReturn(1000000); + $this->estimatorService->expects($this->once()) + ->method('predict') + ->with('user', '1.2.3.4', $this->equalTo(new Ipv4Strategy())) + ->willReturn(false); + $this->mapper->expects($this->at(1)) + ->method('findRelated') + ->with( + 'user', + '1.2.3.4', + $this->anything(), + 1000000-60*60*24*TrainingDataConfig::default()->getThreshold() + ) + ->willReturn([]); + $this->mapper->expects($this->at(2)) + ->method('findRecentByUid') + ->with( + 'user', + 1000000-60*60*24*2 + ) + ->willReturn(array_fill(0, 25, new SuspiciousLogin())); + $this->dispatcher->expects($this->never()) + ->method('dispatch'); + + $this->classifier->process('user', '1.2.3.4'); + } + + public function testProcessHourlyPeakReached(): void { + $this->timeFactory->method('getTime')->willReturn(1000000); + $this->estimatorService->expects($this->once()) + ->method('predict') + ->with('user', '1.2.3.4', $this->equalTo(new Ipv4Strategy())) + ->willReturn(false); + $this->mapper->expects($this->at(2)) + ->method('findRecentByUid') + ->with( + 'user', + 1000000-60*60*24*2 + ) + ->willReturn(array_fill(0, 7, new SuspiciousLogin())); + $this->mapper->expects($this->at(3)) + ->method('findRecentByUid') + ->with( + 'user', + 1000000-60*60 + ) + ->willReturn(array_fill(0, 5, new SuspiciousLogin())); + $this->dispatcher->expects($this->never()) + ->method('dispatch'); + + $this->classifier->process('user', '1.2.3.4'); + } + + public function testProcessNoPeakReached(): void { + $this->timeFactory->method('getTime')->willReturn(1000000); + $this->estimatorService->expects($this->once()) + ->method('predict') + ->with('user', '1.2.3.4', $this->equalTo(new Ipv4Strategy())) + ->willReturn(false); + $this->mapper->expects($this->at(2)) + ->method('findRecentByUid') + ->with( + 'user', + 1000000-60*60*24*2 + ) + ->willReturn(array_fill(0, 7, new SuspiciousLogin())); + $this->mapper->expects($this->at(3)) + ->method('findRecentByUid') + ->with( + 'user', + 1000000-60*60 + ) + ->willReturn(array_fill(0, 1, new SuspiciousLogin())); + $this->dispatcher->expects($this->once()) + ->method('dispatch'); + + $this->classifier->process('user', '1.2.3.4'); + } + +}