Add simple peak detection

lib/Db/SuspiciousLoginMapper.php

@@ -33,14 +33,27 @@ 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
 			->select('*')
 			->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);

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;
 		}
 

tests/Unit/Service/LoginClassifierTest.php

@@ -0,0 +1,184 @@
+<?php declare(strict_types=1);
+
+/**
+ * @copyright 2019 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @author 2019 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ */
+
+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');
+	}
+
+}