Merge pull request #39012 from fsamapoor/refactor_lib_private_security_part2

[2/3] Refactors lib/private/Security

lib/private/Security/IdentityProof/Key.php

@@ -27,18 +27,10 @@ declare(strict_types=1);
 namespace OC\Security\IdentityProof;
 
 class Key {
-	/** @var string */
-	private $publicKey;
-	/** @var string */
-	private $privateKey;
-
-	/**
-	 * @param string $publicKey
-	 * @param string $privateKey
-	 */
-	public function __construct(string $publicKey, string $privateKey) {
-		$this->publicKey = $publicKey;
-		$this->privateKey = $privateKey;
+	public function __construct(
+		private string $publicKey,
+		private string $privateKey,
+	) {
 	}
 
 	public function getPrivate(): string {

lib/private/Security/IdentityProof/Manager.php

@@ -37,23 +37,15 @@ use OCP\Security\ICrypto;
 use Psr\Log\LoggerInterface;
 
 class Manager {
-	/** @var IAppData */
-	private $appData;
-	/** @var ICrypto */
-	private $crypto;
-	/** @var IConfig */
-	private $config;
-	private LoggerInterface $logger;
-
-	public function __construct(Factory $appDataFactory,
-								ICrypto $crypto,
-								IConfig $config,
-								LoggerInterface $logger
+	private IAppData $appData;
+
+	public function __construct(
+		Factory $appDataFactory,
+		private ICrypto $crypto,
+		private IConfig $config,
+		private LoggerInterface $logger,
 	) {
 		$this->appData = $appDataFactory->get('identityproof');
-		$this->crypto = $crypto;
-		$this->config = $config;
-		$this->logger = $logger;
 	}
 
 	/**
@@ -94,7 +86,6 @@ class Manager {
 	 * Note: If a key already exists it will be overwritten
 	 *
 	 * @param string $id key id
-	 * @return Key
 	 * @throws \RuntimeException
 	 */
 	protected function generateKey(string $id): Key {
@@ -117,8 +108,6 @@ class Manager {
 	/**
 	 * Get key for a specific id
 	 *
-	 * @param string $id
-	 * @return Key
 	 * @throws \RuntimeException
 	 */
 	protected function retrieveKey(string $id): Key {
@@ -137,8 +126,6 @@ class Manager {
 	/**
 	 * Get public and private key for $user
 	 *
-	 * @param IUser $user
-	 * @return Key
 	 * @throws \RuntimeException
 	 */
 	public function getKey(IUser $user): Key {
@@ -149,7 +136,6 @@ class Manager {
 	/**
 	 * Get instance wide public and private key
 	 *
-	 * @return Key
 	 * @throws \RuntimeException
 	 */
 	public function getSystemKey(): Key {

lib/private/Security/IdentityProof/Signer.php

@@ -32,32 +32,16 @@ use OCP\IUser;
 use OCP\IUserManager;
 
 class Signer {
-	/** @var Manager */
-	private $keyManager;
-	/** @var ITimeFactory */
-	private $timeFactory;
-	/** @var IUserManager */
-	private $userManager;
-
-	/**
-	 * @param Manager $keyManager
-	 * @param ITimeFactory $timeFactory
-	 * @param IUserManager $userManager
-	 */
-	public function __construct(Manager $keyManager,
-								ITimeFactory $timeFactory,
-								IUserManager $userManager) {
-		$this->keyManager = $keyManager;
-		$this->timeFactory = $timeFactory;
-		$this->userManager = $userManager;
+	public function __construct(
+		private Manager $keyManager,
+		private ITimeFactory $timeFactory,
+		private IUserManager $userManager,
+	) {
 	}
 
 	/**
 	 * Returns a signed blob for $data
 	 *
-	 * @param string $type
-	 * @param array $data
-	 * @param IUser $user
 	 * @return array ['message', 'signature']
 	 */
 	public function sign(string $type, array $data, IUser $user): array {
@@ -79,13 +63,10 @@ class Signer {
 	/**
 	 * Whether the data is signed properly
 	 *
-	 * @param array $data
-	 * @return bool
 	 */
 	public function verify(array $data): bool {
-		if (isset($data['message'])
+		if (isset($data['message']['signer'])
 			&& isset($data['signature'])
-			&& isset($data['message']['signer'])
 		) {
 			$location = strrpos($data['message']['signer'], '@');
 			$userId = substr($data['message']['signer'], 0, $location);

lib/private/Security/RateLimiting/Backend/DatabaseBackend.php

@@ -28,6 +28,7 @@ declare(strict_types=1);
 namespace OC\Security\RateLimiting\Backend;
 
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\DB\Exception;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IConfig;
 use OCP\IDBConnection;
@@ -35,38 +36,22 @@ use OCP\IDBConnection;
 class DatabaseBackend implements IBackend {
 	private const TABLE_NAME = 'ratelimit_entries';
 
-	/** @var IConfig */
-	private $config;
-	/** @var IDBConnection */
-	private $dbConnection;
-	/** @var ITimeFactory */
-	private $timeFactory;
-
 	public function __construct(
-		IConfig $config,
-		IDBConnection $dbConnection,
-		ITimeFactory $timeFactory
+		private IConfig $config,
+		private IDBConnection $dbConnection,
+		private ITimeFactory $timeFactory
 	) {
-		$this->config = $config;
-		$this->dbConnection = $dbConnection;
-		$this->timeFactory = $timeFactory;
 	}
 
-	/**
-	 * @param string $methodIdentifier
-	 * @param string $userIdentifier
-	 * @return string
-	 */
-	private function hash(string $methodIdentifier,
-						  string $userIdentifier): string {
+	private function hash(
+		string $methodIdentifier,
+		string $userIdentifier,
+	): string {
 		return hash('sha512', $methodIdentifier . $userIdentifier);
 	}
 
 	/**
-	 * @param string $identifier
-	 * @param int $seconds
-	 * @return int
-	 * @throws \OCP\DB\Exception
+	 * @throws Exception
 	 */
 	private function getExistingAttemptCount(
 		string $identifier
@@ -97,8 +82,10 @@ class DatabaseBackend implements IBackend {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function getAttempts(string $methodIdentifier,
-								string $userIdentifier): int {
+	public function getAttempts(
+		string $methodIdentifier,
+		string $userIdentifier,
+	): int {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		return $this->getExistingAttemptCount($identifier);
 	}
@@ -106,9 +93,11 @@ class DatabaseBackend implements IBackend {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function registerAttempt(string $methodIdentifier,
-									string $userIdentifier,
-									int $period) {
+	public function registerAttempt(
+		string $methodIdentifier,
+		string $userIdentifier,
+		int $period,
+	): void {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		$deleteAfter = $this->timeFactory->getDateTime()->add(new \DateInterval("PT{$period}S"));
 

lib/private/Security/RateLimiting/Backend/IBackend.php

@@ -39,10 +39,11 @@ interface IBackend {
 	 *
 	 * @param string $methodIdentifier Identifier for the method
 	 * @param string $userIdentifier Identifier for the user
-	 * @return int
 	 */
-	public function getAttempts(string $methodIdentifier,
-								string $userIdentifier): int;
+	public function getAttempts(
+		string $methodIdentifier,
+		string $userIdentifier,
+	): int;
 
 	/**
 	 * Registers an attempt
@@ -51,7 +52,9 @@ interface IBackend {
 	 * @param string $userIdentifier Identifier for the user
 	 * @param int $period Period in seconds how long this attempt should be stored
 	 */
-	public function registerAttempt(string $methodIdentifier,
-									string $userIdentifier,
-									int $period);
+	public function registerAttempt(
+		string $methodIdentifier,
+		string $userIdentifier,
+		int $period,
+	);
 }

lib/private/Security/RateLimiting/Backend/MemoryCacheBackend.php

@@ -42,36 +42,23 @@ use OCP\IConfig;
  * @package OC\Security\RateLimiting\Backend
  */
 class MemoryCacheBackend implements IBackend {
-	/** @var IConfig */
-	private $config;
-	/** @var ICache */
-	private $cache;
-	/** @var ITimeFactory */
-	private $timeFactory;
+	private ICache $cache;
 
 	public function __construct(
-		IConfig $config,
+		private IConfig $config,
 		ICacheFactory $cacheFactory,
-		ITimeFactory $timeFactory) {
-		$this->config = $config;
+		private ITimeFactory $timeFactory,
+	) {
 		$this->cache = $cacheFactory->createDistributed(__CLASS__);
-		$this->timeFactory = $timeFactory;
 	}
 
-	/**
-	 * @param string $methodIdentifier
-	 * @param string $userIdentifier
-	 * @return string
-	 */
-	private function hash(string $methodIdentifier,
-						  string $userIdentifier): string {
+	private function hash(
+		string $methodIdentifier,
+		string $userIdentifier,
+	): string {
 		return hash('sha512', $methodIdentifier . $userIdentifier);
 	}
 
-	/**
-	 * @param string $identifier
-	 * @return array
-	 */
 	private function getExistingAttempts(string $identifier): array {
 		$cachedAttempts = $this->cache->get($identifier);
 		if ($cachedAttempts === null) {
@@ -89,8 +76,10 @@ class MemoryCacheBackend implements IBackend {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function getAttempts(string $methodIdentifier,
-								string $userIdentifier): int {
+	public function getAttempts(
+		string $methodIdentifier,
+		string $userIdentifier,
+	): int {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		$existingAttempts = $this->getExistingAttempts($identifier);
 
@@ -108,9 +97,11 @@ class MemoryCacheBackend implements IBackend {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function registerAttempt(string $methodIdentifier,
-									string $userIdentifier,
-									int $period) {
+	public function registerAttempt(
+		string $methodIdentifier,
+		string $userIdentifier,
+		int $period,
+	): void {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		$existingAttempts = $this->getExistingAttempts($identifier);
 		$currentTime = $this->timeFactory->getTime();

lib/private/Security/RateLimiting/Limiter.php

@@ -32,27 +32,21 @@ use OC\Security\RateLimiting\Exception\RateLimitExceededException;
 use OCP\IUser;
 
 class Limiter {
-	/** @var IBackend */
-	private $backend;
-
-	/**
-	 * @param IBackend $backend
-	 */
-	public function __construct(IBackend $backend) {
-		$this->backend = $backend;
+	public function __construct(
+		private IBackend $backend,
+	) {
 	}
 
 	/**
-	 * @param string $methodIdentifier
-	 * @param string $userIdentifier
 	 * @param int $period in seconds
-	 * @param int $limit
 	 * @throws RateLimitExceededException
 	 */
-	private function register(string $methodIdentifier,
-							  string $userIdentifier,
-							  int $period,
-							  int $limit): void {
+	private function register(
+		string $methodIdentifier,
+		string $userIdentifier,
+		int $period,
+		int $limit,
+	): void {
 		$existingAttempts = $this->backend->getAttempts($methodIdentifier, $userIdentifier);
 		if ($existingAttempts >= $limit) {
 			throw new RateLimitExceededException();
@@ -64,16 +58,15 @@ class Limiter {
 	/**
 	 * Registers attempt for an anonymous request
 	 *
-	 * @param string $identifier
-	 * @param int $anonLimit
 	 * @param int $anonPeriod in seconds
-	 * @param string $ip
 	 * @throws RateLimitExceededException
 	 */
-	public function registerAnonRequest(string $identifier,
-										int $anonLimit,
-										int $anonPeriod,
-										string $ip): void {
+	public function registerAnonRequest(
+		string $identifier,
+		int $anonLimit,
+		int $anonPeriod,
+		string $ip,
+	): void {
 		$ipSubnet = (new IpAddress($ip))->getSubnet();
 
 		$anonHashIdentifier = hash('sha512', 'anon::' . $identifier . $ipSubnet);
@@ -83,16 +76,15 @@ class Limiter {
 	/**
 	 * Registers attempt for an authenticated request
 	 *
-	 * @param string $identifier
-	 * @param int $userLimit
 	 * @param int $userPeriod in seconds
-	 * @param IUser $user
 	 * @throws RateLimitExceededException
 	 */
-	public function registerUserRequest(string $identifier,
-										int $userLimit,
-										int $userPeriod,
-										IUser $user): void {
+	public function registerUserRequest(
+		string $identifier,
+		int $userLimit,
+		int $userPeriod,
+		IUser $user,
+	): void {
 		$userHashIdentifier = hash('sha512', 'user::' . $identifier . $user->getUID());
 		$this->register($identifier, $userHashIdentifier, $userPeriod, $userLimit);
 	}

lib/private/Security/VerificationToken/CleanUpJob.php

@@ -39,18 +39,17 @@ class CleanUpJob extends Job {
 	protected ?string $userId = null;
 	protected ?string $subject = null;
 	protected ?string $pwdPrefix = null;
-	private IConfig $config;
-	private IVerificationToken $verificationToken;
-	private IUserManager $userManager;
 
-	public function __construct(ITimeFactory $time, IConfig $config, IVerificationToken $verificationToken, IUserManager $userManager) {
+	public function __construct(
+		ITimeFactory $time,
+		private IConfig $config,
+		private IVerificationToken $verificationToken,
+		private IUserManager $userManager,
+	) {
 		parent::__construct($time);
-		$this->config = $config;
-		$this->verificationToken = $verificationToken;
-		$this->userManager = $userManager;
 	}
 
-	public function setArgument($argument) {
+	public function setArgument($argument): void {
 		parent::setArgument($argument);
 		$args = \json_decode($argument, true);
 		$this->userId = (string)$args['userId'];
@@ -59,7 +58,7 @@ class CleanUpJob extends Job {
 		$this->runNotBefore = (int)$args['notBefore'];
 	}
 
-	protected function run($argument) {
+	protected function run($argument): void {
 		try {
 			$user = $this->userManager->get($this->userId);
 			if ($user === null) {

lib/private/Security/VerificationToken/VerificationToken.php

@@ -39,29 +39,13 @@ use function json_encode;
 class VerificationToken implements IVerificationToken {
 	protected const TOKEN_LIFETIME = 60 * 60 * 24 * 7;
 
-	/** @var IConfig */
-	private $config;
-	/** @var ICrypto */
-	private $crypto;
-	/** @var ITimeFactory */
-	private $timeFactory;
-	/** @var ISecureRandom */
-	private $secureRandom;
-	/** @var IJobList */
-	private $jobList;
-
 	public function __construct(
-		IConfig $config,
-		ICrypto $crypto,
-		ITimeFactory $timeFactory,
-		ISecureRandom $secureRandom,
-		IJobList $jobList
+		private IConfig $config,
+		private ICrypto $crypto,
+		private ITimeFactory $timeFactory,
+		private ISecureRandom $secureRandom,
+		private IJobList $jobList
 	) {
-		$this->config = $config;
-		$this->crypto = $crypto;
-		$this->timeFactory = $timeFactory;
-		$this->secureRandom = $secureRandom;
-		$this->jobList = $jobList;
 	}
 
 	/**
@@ -71,7 +55,13 @@ class VerificationToken implements IVerificationToken {
 		throw new InvalidTokenException($code);
 	}
 
-	public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = '', bool $expiresWithLogin = false): void {
+	public function check(
+		string $token,
+		?IUser $user,
+		string $subject,
+		string $passwordPrefix = '',
+		bool $expiresWithLogin = false,
+	): void {
 		if ($user === null || !$user->isEnabled()) {
 			$this->throwInvalidTokenException(InvalidTokenException::USER_UNKNOWN);
 		}
@@ -107,7 +97,11 @@ class VerificationToken implements IVerificationToken {
 		}
 	}
 
-	public function create(IUser $user, string $subject, string $passwordPrefix = ''): string {
+	public function create(
+		IUser $user,
+		string $subject,
+		string $passwordPrefix = '',
+	): string {
 		$token = $this->secureRandom->generate(
 			21,
 			ISecureRandom::CHAR_DIGITS.