Merge pull request #39013 from fsamapoor/refactor_lib_private_securit…

…y_part3 [3/3] Refactors lib/private/Security
nextcloud · Sep 22, 2023 · 6b767e0 · 6b767e0
2 parents b6927d0 + 1c023e6
commit 6b767e0
Show file tree

Hide file tree

Showing 9 changed files with 53 additions and 152 deletions.
diff --git a/lib/private/Security/Certificate.php b/lib/private/Security/Certificate.php
@@ -30,25 +30,23 @@
 use OCP\ICertificate;
 
 class Certificate implements ICertificate {
-	protected $name;
+	protected string $name;
 
-	protected $commonName;
+	protected ?string $commonName;
 
-	protected $organization;
+	protected ?string $organization;
 
-	protected $serial;
 
-	protected $issueDate;
+	protected \DateTime $issueDate;
 
-	protected $expireDate;
+	protected \DateTime $expireDate;
 
-	protected $issuerName;
+	protected ?string $issuerName;
 
-	protected $issuerOrganization;
+	protected ?string $issuerOrganization;
 
 	/**
 	 * @param string $data base64 encoded certificate
-	 * @param string $name
 	 * @throws \Exception If the certificate could not get parsed
 	 */
 	public function __construct(string $data, string $name) {
@@ -66,67 +64,43 @@ public function __construct(string $data, string $name) {
 			throw new \Exception('Certificate could not get parsed.');
 		}
 
-		$this->commonName = isset($info['subject']['CN']) ? $info['subject']['CN'] : null;
-		$this->organization = isset($info['subject']['O']) ? $info['subject']['O'] : null;
+		$this->commonName = $info['subject']['CN'] ?? null;
+		$this->organization = $info['subject']['O'] ?? null;
 		$this->issueDate = new \DateTime('@' . $info['validFrom_time_t'], $gmt);
 		$this->expireDate = new \DateTime('@' . $info['validTo_time_t'], $gmt);
-		$this->issuerName = isset($info['issuer']['CN']) ? $info['issuer']['CN'] : null;
-		$this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null;
+		$this->issuerName = $info['issuer']['CN'] ?? null;
+		$this->issuerOrganization = $info['issuer']['O'] ?? null;
 	}
 
-	/**
-	 * @return string
-	 */
 	public function getName(): string {
 		return $this->name;
 	}
 
-	/**
-	 * @return string|null
-	 */
 	public function getCommonName(): ?string {
 		return $this->commonName;
 	}
 
-	/**
-	 * @return string|null
-	 */
 	public function getOrganization(): ?string {
 		return $this->organization;
 	}
 
-	/**
-	 * @return \DateTime
-	 */
 	public function getIssueDate(): \DateTime {
 		return $this->issueDate;
 	}
 
-	/**
-	 * @return \DateTime
-	 */
 	public function getExpireDate(): \DateTime {
 		return $this->expireDate;
 	}
 
-	/**
-	 * @return bool
-	 */
 	public function isExpired(): bool {
 		$now = new \DateTime();
 		return $this->issueDate > $now or $now > $this->expireDate;
 	}
 
-	/**
-	 * @return string|null
-	 */
 	public function getIssuerName(): ?string {
 		return $this->issuerName;
 	}
 
-	/**
-	 * @return string|null
-	 */
 	public function getIssuerOrganization(): ?string {
 		return $this->issuerOrganization;
 	}

diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php
@@ -44,21 +44,14 @@
  * Manage trusted certificates for users
  */
 class CertificateManager implements ICertificateManager {
-	protected View $view;
-	protected IConfig $config;
-	protected LoggerInterface $logger;
-	protected ISecureRandom $random;
-
 	private ?string $bundlePath = null;
 
-	public function __construct(View $view,
-								IConfig $config,
-								LoggerInterface $logger,
-								ISecureRandom $random) {
-		$this->view = $view;
-		$this->config = $config;
-		$this->logger = $logger;
-		$this->random = $random;
+	public function __construct(
+		protected View $view,
+		protected IConfig $config,
+		protected LoggerInterface $logger,
+		protected ISecureRandom $random,
+	) {
 	}
 
 	/**
@@ -178,7 +171,6 @@ public function createCertificateBundle(): void {
 	 *
 	 * @param string $certificate the certificate data
 	 * @param string $name the filename for the certificate
-	 * @return \OCP\ICertificate
 	 * @throws \Exception If the certificate could not get added
 	 */
 	public function addCertificate(string $certificate, string $name): ICertificate {
@@ -205,9 +197,6 @@ public function addCertificate(string $certificate, string $name): ICertificate
 
 	/**
 	 * Remove the certificate and re-generate the certificate bundle
-	 *
-	 * @param string $name
-	 * @return bool
 	 */
 	public function removeCertificate(string $name): bool {
 		if (!Filesystem::isValidPath($name)) {
@@ -225,8 +214,6 @@ public function removeCertificate(string $name): bool {
 
 	/**
 	 * Get the path to the certificate bundle
-	 *
-	 * @return string
 	 */
 	public function getCertificateBundle(): string {
 		return $this->getPathToCertificates() . 'rootcerts.crt';
@@ -267,8 +254,6 @@ private function getPathToCertificates(): string {
 
 	/**
 	 * Check if we need to re-bundle the certificates because one of the sources has updated
-	 *
-	 * @return bool
 	 */
 	private function needsRebundling(): bool {
 		$targetBundle = $this->getCertificateBundle();
@@ -282,8 +267,6 @@ private function needsRebundling(): bool {
 
 	/**
 	 * get mtime of ca-bundle shipped by Nextcloud
-	 *
-	 * @return int
 	 */
 	protected function getFilemtimeOfCaBundle(): int {
 		return filemtime(\OC::$SERVERROOT . '/resources/config/ca-bundle.crt');

diff --git a/lib/private/Security/CredentialsManager.php b/lib/private/Security/CredentialsManager.php
@@ -40,26 +40,16 @@
 class CredentialsManager implements ICredentialsManager {
 	public const DB_TABLE = 'storages_credentials';
 
-	/** @var ICrypto */
-	protected $crypto;
-
-	/** @var IDBConnection */
-	protected $dbConnection;
-
-	/**
-	 * @param ICrypto $crypto
-	 * @param IDBConnection $dbConnection
-	 */
-	public function __construct(ICrypto $crypto, IDBConnection $dbConnection) {
-		$this->crypto = $crypto;
-		$this->dbConnection = $dbConnection;
+	public function __construct(
+		protected ICrypto $crypto,
+		protected IDBConnection $dbConnection,
+	) {
 	}
 
 	/**
 	 * Store a set of credentials
 	 *
 	 * @param string $userId empty string for system-wide credentials
-	 * @param string $identifier
 	 * @param mixed $credentials
 	 */
 	public function store(string $userId, string $identifier, $credentials): void {
@@ -77,10 +67,8 @@ public function store(string $userId, string $identifier, $credentials): void {
 	 * Retrieve a set of credentials
 	 *
 	 * @param string $userId empty string for system-wide credentials
-	 * @param string $identifier
-	 * @return mixed
 	 */
-	public function retrieve(string $userId, string $identifier) {
+	public function retrieve(string $userId, string $identifier): mixed {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->select('credentials')
 			->from(self::DB_TABLE)
@@ -108,7 +96,6 @@ public function retrieve(string $userId, string $identifier) {
 	 * Delete a set of credentials
 	 *
 	 * @param string $userId empty string for system-wide credentials
-	 * @param string $identifier
 	 * @return int rows removed
 	 */
 	public function delete(string $userId, string $identifier): int {
@@ -128,7 +115,6 @@ public function delete(string $userId, string $identifier): int {
 	/**
 	 * Erase all credentials stored for a user
 	 *
-	 * @param string $userId
 	 * @return int rows removed
 	 */
 	public function erase(string $userId): int {

diff --git a/lib/private/Security/Crypto.php b/lib/private/Security/Crypto.php
@@ -32,7 +32,6 @@
 use Exception;
 use OCP\IConfig;
 use OCP\Security\ICrypto;
-use OCP\Security\ISecureRandom;
 use phpseclib\Crypt\AES;
 use phpseclib\Crypt\Hash;
 
@@ -47,20 +46,13 @@
  * @package OC\Security
  */
 class Crypto implements ICrypto {
-	/** @var AES $cipher */
-	private $cipher;
-	/** @var int */
-	private $ivLength = 16;
-	/** @var IConfig */
-	private $config;
+	private AES $cipher;
+	private int $ivLength = 16;
 
-	/**
-	 * @param IConfig $config
-	 * @param ISecureRandom $random
-	 */
-	public function __construct(IConfig $config) {
+	public function __construct(
+		private IConfig $config,
+	) {
 		$this->cipher = new AES();
-		$this->config = $config;
 	}
 
 	/**
@@ -84,7 +76,6 @@ public function calculateHMAC(string $message, string $password = ''): string {
 	/**
 	 * Encrypts a value and adds an HMAC (Encrypt-Then-MAC)
 	 *
-	 * @param string $plaintext
 	 * @param string $password Password to encrypt, if not specified the secret from config.php will be taken
 	 * @return string Authenticated ciphertext
 	 * @throws Exception if it was not possible to gather sufficient entropy
@@ -115,9 +106,7 @@ public function encrypt(string $plaintext, string $password = ''): string {
 
 	/**
 	 * Decrypts a value and verifies the HMAC (Encrypt-Then-Mac)
-	 * @param string $authenticatedCiphertext
 	 * @param string $password Password to encrypt, if not specified the secret from config.php will be taken
-	 * @return string plaintext
 	 * @throws Exception If the HMAC does not match
 	 * @throws Exception If the decryption failed
 	 */

diff --git a/lib/private/Security/Hasher.php b/lib/private/Security/Hasher.php
@@ -51,19 +51,14 @@
  * @package OC\Security
  */
 class Hasher implements IHasher {
-	/** @var IConfig */
-	private $config;
-	/** @var array Options passed to password_hash and password_needs_rehash */
-	private $options = [];
-	/** @var string Salt used for legacy passwords */
-	private $legacySalt = null;
-
-	/**
-	 * @param IConfig $config
-	 */
-	public function __construct(IConfig $config) {
-		$this->config = $config;
-
+	/** Options passed to password_hash and password_needs_rehash */
+	private array $options = [];
+	/** Salt used for legacy passwords */
+	private ?string $legacySalt = null;
+
+	public function __construct(
+		private IConfig $config,
+	) {
 		if (\defined('PASSWORD_ARGON2ID') || \defined('PASSWORD_ARGON2I')) {
 			// password_hash fails, when the minimum values are undershot.
 			// In this case, apply minimum.
@@ -106,7 +101,7 @@ public function hash(string $message): string {
 	 * @param string $prefixedHash
 	 * @return null|array Null if the hash is not prefixed, otherwise array('version' => 1, 'hash' => 'foo')
 	 */
-	protected function splitHash(string $prefixedHash) {
+	protected function splitHash(string $prefixedHash): ?array {
 		$explodedString = explode('|', $prefixedHash, 2);
 		if (\count($explodedString) === 2) {
 			if ((int)$explodedString[0] > 0) {
@@ -198,7 +193,7 @@ private function needsRehash(string $hash): bool {
 		return password_needs_rehash($hash, $algorithm, $this->options);
 	}
 
-	private function getPrefferedAlgorithm() {
+	private function getPrefferedAlgorithm(): string {
 		$default = PASSWORD_BCRYPT;
 		if (\defined('PASSWORD_ARGON2I')) {
 			$default = PASSWORD_ARGON2I;

diff --git a/lib/private/Security/Normalizer/IpAddress.php b/lib/private/Security/Normalizer/IpAddress.php
@@ -37,22 +37,16 @@
  * @package OC\Security\Normalizer
  */
 class IpAddress {
-	/** @var string */
-	private $ip;
-
 	/**
 	 * @param string $ip IP to normalized
 	 */
-	public function __construct(string $ip) {
-		$this->ip = $ip;
+	public function __construct(
+		private string $ip,
+	) {
 	}
 
 	/**
 	 * Return the given subnet for an IPv4 address and mask bits
-	 *
-	 * @param string $ip
-	 * @param int $maskBits
-	 * @return string
 	 */
 	private function getIPv4Subnet(string $ip, int $maskBits = 32): string {
 		$binary = \inet_pton($ip);
@@ -68,10 +62,6 @@ private function getIPv4Subnet(string $ip, int $maskBits = 32): string {
 
 	/**
 	 * Return the given subnet for an IPv6 address and mask bits
-	 *
-	 * @param string $ip
-	 * @param int $maskBits
-	 * @return string
 	 */
 	private function getIPv6Subnet(string $ip, int $maskBits = 48): string {
 		if ($ip[0] === '[' && $ip[-1] === ']') { // If IP is with brackets, for example [::1]
@@ -126,8 +116,6 @@ private function getEmbeddedIpv4(string $ipv6): ?string {
 
 	/**
 	 * Gets either the /32 (IPv4) or the /64 (IPv6) subnet of an IP address
-	 *
-	 * @return string
 	 */
 	public function getSubnet(): string {
 		if (\preg_match('/^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$/', $this->ip)) {
@@ -153,8 +141,6 @@ public function getSubnet(): string {
 
 	/**
 	 * Returns the specified IP address
-	 *
-	 * @return string
 	 */
 	public function __toString(): string {
 		return $this->ip;