From 820670da8e2c544b6d5cad2757177253e8e5af6b Mon Sep 17 00:00:00 2001 From: Avinash Gusain Date: Fri, 13 Jun 2025 19:40:00 +0530 Subject: [PATCH 1/6] Fix:Reused old service with proper error handling --- lib/Command/SpamAccountDetection.php | 10 +- lib/Service/RecoveryEmailService.php | 31 +++++ lib/Service/SpamEmailService.php | 198 --------------------------- 3 files changed, 36 insertions(+), 203 deletions(-) delete mode 100644 lib/Service/SpamEmailService.php diff --git a/lib/Command/SpamAccountDetection.php b/lib/Command/SpamAccountDetection.php index 82eeb16..b82929c 100644 --- a/lib/Command/SpamAccountDetection.php +++ b/lib/Command/SpamAccountDetection.php @@ -5,7 +5,7 @@ declare(strict_types=1); namespace OCA\EmailRecovery\Command; use OCA\EmailRecovery\AppInfo\Application; -use OCA\EmailRecovery\Service\SpamEmailService; +use OCA\EmailRecovery\Service\RecoveryEmailService; use OCP\Files\IAppData; use OCP\ILogger; use Symfony\Component\Console\Command\Command; @@ -13,13 +13,13 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; class SpamAccountDetection extends Command { - private SpamEmailService $spamEmailService; + private RecoveryEmailService $recoveryEmailService; private ILogger $logger; private IAppData $appData; - public function __construct(SpamEmailService $spamEmailService, ILogger $logger, IAppData $appData) { + public function __construct(RecoveryEmailService $recoveryEmailService, ILogger $logger, IAppData $appData) { parent::__construct(); - $this->spamEmailService = $spamEmailService; + $this->recoveryEmailService = $recoveryEmailService; $this->logger = $logger; $this->appData = $appData; } @@ -32,7 +32,7 @@ class SpamAccountDetection extends Command { protected function execute(InputInterface $input, OutputInterface $output): int { try { - $spamUserNames = $this->spamEmailService->getAllSpamEmails(); + $spamUserNames = $this->recoveryEmailService->getAllSpamEmails(); $output->writeln('Spam user list:'); foreach ($spamUserNames as $username) { diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index d4b7c7f..49e257c 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -560,4 +560,35 @@ class RecoveryEmailService { $requests[] = $now; $this->cache->set($key, $requests, 2); } + + public function getAllSpamEmails(): array { + $verifiedEmails = $this->configMapper->getAllVerifiedRecoveryEmails(); + $spamEmails = []; + foreach ($verifiedEmails as $entry) { + $recoveryEmail = strtolower(trim($entry['configvalue'])); + $userId = strtolower(trim($entry['userid'])); + if ($recoveryEmail !== '' && $userId !== '') { + try { + if (!$this->validateRecoveryEmail($recoveryEmail, $userId)) { + $spamEmails[] = $userId; + } + } catch (\RuntimeException $e) { + $msg = $e->getMessage(); + + if (str_starts_with($msg, 'Invalid response received while verifying domain')) { + $this->logger->info("Domain check failed for $userId <$recoveryEmail>: " . $msg); + } elseif (str_starts_with($msg, 'The email could not be verified')) { + $this->logger->info("Email unverifiable for $userId <$recoveryEmail>: " . $msg); + } else { + $this->logger->info("RuntimeException for $userId <$recoveryEmail>: " . $msg); + } + } catch (\Throwable $e) { + // Catch all other exceptions + $this->logger->info("Unexpected error for $userId <$recoveryEmail>: " . $e->getMessage()); + $spamEmails[] = $userId; + } + } + } + return $spamEmails; + } } diff --git a/lib/Service/SpamEmailService.php b/lib/Service/SpamEmailService.php deleted file mode 100644 index edaaa7c..0000000 --- a/lib/Service/SpamEmailService.php +++ /dev/null @@ -1,198 +0,0 @@ -logger = $logger; - $this->config = $config; - $this->userManager = $userManager; - $this->l10nFactory = $l10nFactory; - $this->httpClientService = $httpClientService; - $this->cacheFactory = $cacheFactory; - $this->domainService = $domainService; - $this->l = $l; - $this->configMapper = $configMapper; - $this->cache = $cacheFactory->createDistributed(self::CACHE_KEY); - } - - public function getAllSpamEmails(): array { - $verifiedEmails = $this->configMapper->getAllVerifiedRecoveryEmails(); - $spamEmails = []; - foreach ($verifiedEmails as $entry) { - $recoveryEmail = strtolower(trim($entry['configvalue'])); - $userId = strtolower(trim($entry['userid'])); - if ($recoveryEmail !== '' && $userId !== '') { - try { - if (!$this->validateRecoveryEmail($recoveryEmail, $userId)) { - $spamEmails[] = $userId; - } - } catch (\Throwable $e) { - $this->logger->info("Validation failed for $userId <$recoveryEmail>: " . $e->getMessage()); - } - } - } - return $spamEmails; - } - - public function validateRecoveryEmail(string $recoveryEmail, string $username = '', string $language = 'en'): bool { - if (empty($recoveryEmail)) { - return true; - } - - $email = $this->getUserEmail($username); - $l = $this->l10nFactory->get(Application::APP_ID, $language); - if (!$this->enforceBasicRecoveryEmailRules($recoveryEmail, $username, $email, $l)) { - return false; - } - - $apiKey = $this->config->getSystemValue('verify_mail_api_key', ''); - if ($this->domainService->isDomainInCustomBlacklist($recoveryEmail, $l)) { - return false; - } - - $domainCheckPassed = true; - $emailCheckPassed = true; - - if ($this->domainService->isPopularDomain($recoveryEmail, $l)) { - $emailCheckPassed = $this->ensureRealTimeRateLimit(self::RATE_LIMIT_EMAIL, 2, $l); - } else { - $domainCheckPassed = $this->ensureRealTimeRateLimit(self::RATE_LIMIT_DOMAIN, 15, $l); - if ($domainCheckPassed) { - $domain = substr(strrchr($recoveryEmail, "@"), 1); - try { - $this->verifyDomainWithApi($domain, $username, $apiKey, $l); - } catch (\Throwable $e) { - $this->logger->info("Domain verification failed for $username <$recoveryEmail>: " . $e->getMessage()); - return false; - } - $emailCheckPassed = $this->ensureRealTimeRateLimit(self::RATE_LIMIT_EMAIL, 2, $l); - } - } - - if (!$emailCheckPassed) { - return false; - } - - return $this->ensureEmailIsValid($recoveryEmail, $username, $apiKey, $l); - } - - private function getUserEmail(string $username): string { - if (empty($username)) { - return ''; - } - $user = $this->userManager->get($username); - return $user->getEMailAddress(); - } - - private function enforceBasicRecoveryEmailRules(string $recoveryEmail, string $username, string $email, IL10N $l): bool { - if (!filter_var($recoveryEmail, FILTER_VALIDATE_EMAIL)) { - return false; - } - if (!empty($email) && strcmp($recoveryEmail, $email) === 0) { - return false; - } - if ($this->domainService->isBlacklistedDomain($recoveryEmail, $l)) { - return false; - } - return true; - } - /** - * Ensures that a real-time rate limit is not exceeded for a given key. - * - * This function implements a sliding window rate limiter that allows up to `$rateLimit` - * operations per second for a specific cache key. If the limit is reached, it waits - * until a slot becomes available (based on the oldest timestamp), retrying up to `$maxRetries` times. - * - * The function stores timestamps of each request in a cache and filters them to keep - * only those within the last 1 second. If the number of valid timestamps exceeds the - * allowed rate, the function sleeps for the remaining time until the oldest timestamp - * expires. After retrying for `$maxRetries` times, it gives up and returns `false`. - * - * @param string $key The unique cache key used to track rate-limited actions. - * @param int $rateLimit The maximum number of allowed operations per second. - * @param IL10N $l Localization object - * @param int $maxRetries The maximum number of retry attempts while waiting for a rate slot to become available (default is 10). - * - * @return bool Returns `true` if the rate limit was not exceeded (and the action can proceed), or `false` if retries were exhausted. - */ - - private function ensureRealTimeRateLimit(string $key, int $rateLimit, IL10N $l, int $maxRetries = 10): bool { - $now = microtime(true); - $attempts = 0; - $requests = $this->cache->get($key) ?? []; - $requests = array_filter($requests, fn ($t) => ($now - $t) <= 1); - - while (count($requests) >= $rateLimit) { - usleep((int)((1 - ($now - min($requests))) * 1000000)); - $now = microtime(true); - $requests = array_filter($requests, fn ($t) => ($now - $t) <= 1); - if (++$attempts >= $maxRetries) { - return false; - } - } - - $requests[] = $now; - $this->cache->set($key, $requests, 2); - return true; - } - - private function ensureEmailIsValid(string $recoveryEmail, string $username, string $apiKey, IL10N $l): bool { - $url = sprintf(self::VERIFYMAIL_API_URL, $recoveryEmail, $apiKey); - try { - $httpClient = $this->httpClientService->newClient(); - $response = $httpClient->get($url, ['timeout' => 15]); - $data = json_decode($response->getBody(), true); - if (!is_array($data)) { - return false; - } - if ($data['disposable'] ?? false || !($data['deliverable_email'] ?? true)) { - return false; - } - } catch (\Throwable $e) { - $this->logger->error("Email validation failed for $username <$recoveryEmail>: " . $e->getMessage()); - return true; // fallback to avoid false positive - } - return true; - } - - private function verifyDomainWithApi(string $domain, string $username, string $apiKey, IL10N $l): void { - $url = sprintf(self::VERIFYMAIL_API_URL, $domain, $apiKey); - $httpClient = $this->httpClientService->newClient(); - $response = $httpClient->get($url, ['timeout' => 15]); - $data = json_decode($response->getBody(), true); - if (!$data || !is_array($data)) { - throw new \RuntimeException("Invalid response"); - } - if ($data['disposable'] ?? false || !$data['mx'] ?? true) { - throw new BlacklistedEmailException($l->t('Domain is not valid')); - } - } -} -- GitLab From 564ca6098c4024316379b38f44e9582229cff8af Mon Sep 17 00:00:00 2001 From: Avinash Gusain Date: Mon, 16 Jun 2025 15:24:58 +0530 Subject: [PATCH 2/6] Fix: catched specific exception to add spam emails --- lib/Service/RecoveryEmailService.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index 49e257c..2b34c6f 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -572,6 +572,15 @@ class RecoveryEmailService { if (!$this->validateRecoveryEmail($recoveryEmail, $userId)) { $spamEmails[] = $userId; } + } catch ( + \OCA\EmailRecovery\Exception\BlacklistedEmailException | + \OCA\EmailRecovery\Exception\InvalidRecoveryEmailException | + \OCA\EmailRecovery\Exception\SameRecoveryEmailAsEmailException | + \OCA\EmailRecovery\Exception\RecoveryEmailAlreadyFoundException | + \OCA\EmailRecovery\Exception\MurenaDomainDisallowedException + $e) { + $this->logger->info("Validation failed (spam) for $userId <$recoveryEmail>: " . $e->getMessage()); + $spamEmails[] = $userId; } catch (\RuntimeException $e) { $msg = $e->getMessage(); @@ -585,7 +594,6 @@ class RecoveryEmailService { } catch (\Throwable $e) { // Catch all other exceptions $this->logger->info("Unexpected error for $userId <$recoveryEmail>: " . $e->getMessage()); - $spamEmails[] = $userId; } } } -- GitLab From 52ffdef6bc7a32728f8616e237a5ae6c0c4c1086 Mon Sep 17 00:00:00 2001 From: Avinash Gusain Date: Mon, 16 Jun 2025 15:35:23 +0530 Subject: [PATCH 3/6] Updated the array to pass both username and recovery email --- lib/Command/SpamAccountDetection.php | 7 ++++--- lib/Service/RecoveryEmailService.php | 6 ++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/Command/SpamAccountDetection.php b/lib/Command/SpamAccountDetection.php index b82929c..72e18e6 100644 --- a/lib/Command/SpamAccountDetection.php +++ b/lib/Command/SpamAccountDetection.php @@ -32,11 +32,12 @@ class SpamAccountDetection extends Command { protected function execute(InputInterface $input, OutputInterface $output): int { try { - $spamUserNames = $this->recoveryEmailService->getAllSpamEmails(); + $spamUsers = $this->recoveryEmailService->getAllSpamEmails(); $output->writeln('Spam user list:'); - foreach ($spamUserNames as $username) { - $output->writeln($username); + foreach ($spamUsers as $user) { + $output->writeln($user['userId']); + //$output->writeln("User ID: {$user['userId']}, Recovery Email: {$user['recoveryEmail']}"); } } catch (\Throwable $th) { $this->logger->error('Error while fetching domains. ' . $th->getMessage()); diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index 2b34c6f..f39b505 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -570,7 +570,8 @@ class RecoveryEmailService { if ($recoveryEmail !== '' && $userId !== '') { try { if (!$this->validateRecoveryEmail($recoveryEmail, $userId)) { - $spamEmails[] = $userId; + $spamEmails['userId'] = $userId; + $spamEmails['recoveryEmail'] = $recoveryEmail; } } catch ( \OCA\EmailRecovery\Exception\BlacklistedEmailException | @@ -580,7 +581,8 @@ class RecoveryEmailService { \OCA\EmailRecovery\Exception\MurenaDomainDisallowedException $e) { $this->logger->info("Validation failed (spam) for $userId <$recoveryEmail>: " . $e->getMessage()); - $spamEmails[] = $userId; + $spamEmails['userId'] = $userId; + $spamEmails['recoveryEmail'] = $recoveryEmail; } catch (\RuntimeException $e) { $msg = $e->getMessage(); -- GitLab From 7185d12d50b8bea0c1cb46e7b0b9145bf17132c8 Mon Sep 17 00:00:00 2001 From: Avinash Gusain Date: Mon, 16 Jun 2025 16:10:23 +0530 Subject: [PATCH 4/6] Fix: return correct array --- lib/Service/RecoveryEmailService.php | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index f39b505..7463497 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -563,15 +563,17 @@ class RecoveryEmailService { public function getAllSpamEmails(): array { $verifiedEmails = $this->configMapper->getAllVerifiedRecoveryEmails(); - $spamEmails = []; + $spamAccounts = []; foreach ($verifiedEmails as $entry) { $recoveryEmail = strtolower(trim($entry['configvalue'])); $userId = strtolower(trim($entry['userid'])); if ($recoveryEmail !== '' && $userId !== '') { try { if (!$this->validateRecoveryEmail($recoveryEmail, $userId)) { - $spamEmails['userId'] = $userId; - $spamEmails['recoveryEmail'] = $recoveryEmail; + $spamAccounts[] = [ + 'userId' => $userId, + 'recoveryEmail' => $recoveryEmail, + ]; } } catch ( \OCA\EmailRecovery\Exception\BlacklistedEmailException | @@ -581,8 +583,10 @@ class RecoveryEmailService { \OCA\EmailRecovery\Exception\MurenaDomainDisallowedException $e) { $this->logger->info("Validation failed (spam) for $userId <$recoveryEmail>: " . $e->getMessage()); - $spamEmails['userId'] = $userId; - $spamEmails['recoveryEmail'] = $recoveryEmail; + $spamAccounts[] = [ + 'userId' => $userId, + 'recoveryEmail' => $recoveryEmail, + ]; } catch (\RuntimeException $e) { $msg = $e->getMessage(); @@ -599,6 +603,6 @@ class RecoveryEmailService { } } } - return $spamEmails; + return $spamAccounts; } } -- GitLab From 09cc28c80c3d40f0e0875cf2b3b9235e1e613ea7 Mon Sep 17 00:00:00 2001 From: Avinash Gusain Date: Mon, 16 Jun 2025 18:28:51 +0530 Subject: [PATCH 5/6] Moved all Throwable into one --- lib/Service/RecoveryEmailService.php | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index 7463497..23da109 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -576,30 +576,20 @@ class RecoveryEmailService { ]; } } catch ( - \OCA\EmailRecovery\Exception\BlacklistedEmailException | - \OCA\EmailRecovery\Exception\InvalidRecoveryEmailException | - \OCA\EmailRecovery\Exception\SameRecoveryEmailAsEmailException | - \OCA\EmailRecovery\Exception\RecoveryEmailAlreadyFoundException | - \OCA\EmailRecovery\Exception\MurenaDomainDisallowedException + BlacklistedEmailException | + InvalidRecoveryEmailException | + SameRecoveryEmailAsEmailException | + RecoveryEmailAlreadyFoundException | + MurenaDomainDisallowedException $e) { $this->logger->info("Validation failed (spam) for $userId <$recoveryEmail>: " . $e->getMessage()); $spamAccounts[] = [ 'userId' => $userId, 'recoveryEmail' => $recoveryEmail, ]; - } catch (\RuntimeException $e) { - $msg = $e->getMessage(); - - if (str_starts_with($msg, 'Invalid response received while verifying domain')) { - $this->logger->info("Domain check failed for $userId <$recoveryEmail>: " . $msg); - } elseif (str_starts_with($msg, 'The email could not be verified')) { - $this->logger->info("Email unverifiable for $userId <$recoveryEmail>: " . $msg); - } else { - $this->logger->info("RuntimeException for $userId <$recoveryEmail>: " . $msg); - } } catch (\Throwable $e) { // Catch all other exceptions - $this->logger->info("Unexpected error for $userId <$recoveryEmail>: " . $e->getMessage()); + $this->logger->info("Error while checking $userId <$recoveryEmail>: " . $e->getMessage()); } } } -- GitLab From 858f4364c7bae25439eff79892607bfaf46ac342 Mon Sep 17 00:00:00 2001 From: Avinash Gusain Date: Mon, 16 Jun 2025 21:20:50 +0530 Subject: [PATCH 6/6] added docstring --- lib/Service/RecoveryEmailService.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/lib/Service/RecoveryEmailService.php b/lib/Service/RecoveryEmailService.php index 23da109..0053096 100644 --- a/lib/Service/RecoveryEmailService.php +++ b/lib/Service/RecoveryEmailService.php @@ -561,6 +561,24 @@ class RecoveryEmailService { $this->cache->set($key, $requests, 2); } + /** + * Scans all verified recovery email addresses and returns a list of spam accounts. + * + * This method validates each user's recovery email using a series of checks. + * Users are considered spam if their recovery email is: + * - A disposable address + * - A blacklisted or disallowed domain + * - Invalid in format + * - The same as their account email + * - Already taken by another user + * - Unverifiable or unreachable (e.g., failed MX or domain lookup) + * + * Returns an array of spam account entries, each including: + * - userId: the username + * - recoveryEmail: the flagged recovery email address + * + * @return array + */ public function getAllSpamEmails(): array { $verifiedEmails = $this->configMapper->getAllVerifiedRecoveryEmails(); $spamAccounts = []; -- GitLab