From 84830d6aeff97dcdfccf34bd5edb8ba97f3ec3b2 Mon Sep 17 00:00:00 2001
From: Yuri Kuznetsov <yurikuzn@users.noreply.github.com>
Date: Mon, 30 Aug 2021 12:00:51 +0300
Subject: [PATCH] refactoring

---
 application/Espo/Core/Api/Auth.php            | 52 ++++-------
 .../Core/Authentication/Authentication.php    | 68 +++++++-------
 .../Authentication/AuthenticationData.php     | 93 +++++++++++++++++++
 .../Authentication/AuthenticationFactory.php  |  2 +-
 tests/integration/Core/Tester.php             | 14 +--
 .../Authentication/AuthenticationDataTest.php | 59 ++++++++++++
 6 files changed, 214 insertions(+), 74 deletions(-)
 create mode 100644 application/Espo/Core/Authentication/AuthenticationData.php
 create mode 100644 tests/unit/Espo/Core/Authentication/AuthenticationDataTest.php

diff --git a/application/Espo/Core/Api/Auth.php b/application/Espo/Core/Api/Auth.php
index 548c14c365..127e244a16 100644
--- a/application/Espo/Core/Api/Auth.php
+++ b/application/Espo/Core/Api/Auth.php
@@ -35,6 +35,7 @@ use Espo\Core\Exceptions\ServiceUnavailable;
 use Espo\Core\Api\Request;
 use Espo\Core\Api\Response;
 use Espo\Core\Authentication\Authentication;
+use Espo\Core\Authentication\AuthenticationData;
 use Espo\Core\Authentication\Result;
 use Espo\Core\Utils\Log;
 
@@ -77,15 +78,18 @@ class Auth
             list($username, $password) = $this->obtainUsernamePasswordFromRequest($request);
         }
 
+        $authenticationData = AuthenticationData::create()
+            ->withUsername($username)
+            ->withPassword($password)
+            ->withMethod($authenticationMethod);
+
         $hasAuthData = (bool) ($username || $authenticationMethod);
 
         if (!$this->authRequired && !$this->isEntryPoint && $hasAuthData) {
             $authResult = $this->processAuthNotRequired(
-                $username,
-                $password,
+                $authenticationData,
                 $request,
-                $response,
-                $authenticationMethod
+                $response
             );
 
             if ($authResult) {
@@ -98,13 +102,7 @@ class Auth
         }
 
         if ($hasAuthData) {
-            return $this->processWithAuthData(
-                $username,
-                $password,
-                $request,
-                $response,
-                $authenticationMethod
-            );
+            return $this->processWithAuthData($authenticationData, $request, $response);
         }
 
         $showDialog = $this->isEntryPoint;
@@ -118,22 +116,14 @@ class Auth
         return AuthResult::createNotResolved();
     }
 
-    protected function processAuthNotRequired(
-        string $username,
-        ?string $password,
+    private function processAuthNotRequired(
+        AuthenticationData $data,
         Request $request,
-        Response $response,
-        ?string $authenticationMethod
+        Response $response
     ): ?AuthResult {
 
         try {
-            $result = $this->authentication->login(
-                $username,
-                $password,
-                $request,
-                $response,
-                $authenticationMethod
-            );
+            $result = $this->authentication->login($data, $request, $response);
         }
         catch (Exception $e) {
             $this->handleException($response, $e);
@@ -148,24 +138,16 @@ class Auth
         return null;
     }
 
-    protected function processWithAuthData(
-        ?string $username,
-        ?string $password,
+    private function processWithAuthData(
+        AuthenticationData $data,
         Request $request,
-        Response $response,
-        ?string $authenticationMethod
+        Response $response
     ): AuthResult {
 
         $showDialog = $this->isEntryPoint;
 
         try {
-            $result = $this->authentication->login(
-                $username,
-                $password,
-                $request,
-                $response,
-                $authenticationMethod
-            );
+            $result = $this->authentication->login($data, $request, $response);
         }
         catch (Exception $e) {
             $this->handleException($response, $e);
diff --git a/application/Espo/Core/Authentication/Authentication.php b/application/Espo/Core/Authentication/Authentication.php
index ed597809ad..50e24bde86 100644
--- a/application/Espo/Core/Authentication/Authentication.php
+++ b/application/Espo/Core/Authentication/Authentication.php
@@ -108,38 +108,19 @@ class Authentication
         $this->log = $log;
     }
 
-    protected function setPortal(Portal $portal)
-    {
-        $this->portal = $portal;
-    }
-
-    protected function isPortal(): bool
-    {
-        return (bool) $this->portal || $this->applicationState->isPortal();
-    }
-
-    protected function getPortal(): Portal
-    {
-        if ($this->portal) {
-            return $this->portal;
-        }
-
-        return $this->applicationState->getPortal();
-    }
-
     /**
      * Process logging in.
      *
+     * Warning: This method can change the state of the object (by setting the `portal` prop.).
+     *
      * @throws Forbidden
      * @throws ServiceUnavailable
      */
-    public function login(
-        ?string $username,
-        ?string $password,
-        Request $request,
-        Response $response,
-        ?string $authenticationMethod = null
-    ): Result {
+    public function login(AuthenticationData $data, Request $request, Response $response): Result
+    {
+        $username = $data->getUsername();
+        $password = $data->getPassword();
+        $authenticationMethod = $data->getMethod();
 
         if (
             $authenticationMethod &&
@@ -161,6 +142,12 @@ class Authentication
         $authToken = null;
         $authTokenIsFound = false;
 
+        if (!$authenticationMethod && $password === null) {
+            $this->log->error("AUTH: Trying to login w/o password.");
+
+            return Result::fail('No password');
+        }
+
         if (!$authenticationMethod) {
             $authToken = $this->authTokenManager->get($password);
         }
@@ -239,7 +226,7 @@ class Authentication
         }
 
         if ($this->isPortal()) {
-            $user->set('portalId', $this->getPortal()->id);
+            $user->set('portalId', $this->getPortal()->getId());
         }
 
         if (!$this->isPortal()) {
@@ -300,6 +287,25 @@ class Authentication
         return $result;
     }
 
+    private function setPortal(Portal $portal): void
+    {
+        $this->portal = $portal;
+    }
+
+    private function isPortal(): bool
+    {
+        return (bool) $this->portal || $this->applicationState->isPortal();
+    }
+
+    private function getPortal(): Portal
+    {
+        if ($this->portal) {
+            return $this->portal;
+        }
+
+        return $this->applicationState->getPortal();
+    }
+
     private function processAuthTokenCheck(AuthToken $authToken): bool
     {
         if ($this->allowAnyAccess && $authToken->getPortalId() && !$this->isPortal()) {
@@ -314,7 +320,7 @@ class Authentication
             return true;
         }
 
-        if ($this->isPortal() && $authToken->getPortalId() !== $this->getPortal()->id) {
+        if ($this->isPortal() && $authToken->getPortalId() !== $this->getPortal()->getId()) {
             $this->log->info(
                 "AUTH: Trying to login to portal with a token not related to portal."
             );
@@ -490,7 +496,7 @@ class Authentication
             'hash' => $user->get('password'),
             'ipAddress' => $request->getServerParam('REMOTE_ADDR'),
             'userId' => $user->id,
-            'portalId' => $this->isPortal() ? $this->getPortal()->id : null,
+            'portalId' => $this->isPortal() ? $this->getPortal()->getId() : null,
             'createSecret' => $createSecret,
         ];
 
@@ -547,7 +553,7 @@ class Authentication
         return true;
     }
 
-    protected function createAuthLogRecord(
+    private function createAuthLogRecord(
         ?string $username,
         ?User $user,
         Request $request,
@@ -579,7 +585,7 @@ class Authentication
         ]);
 
         if ($this->isPortal()) {
-            $authLogRecord->set('portalId', $this->getPortal()->id);
+            $authLogRecord->set('portalId', $this->getPortal()->getId());
         }
 
         if ($user) {
diff --git a/application/Espo/Core/Authentication/AuthenticationData.php b/application/Espo/Core/Authentication/AuthenticationData.php
new file mode 100644
index 0000000000..34b92a4dc8
--- /dev/null
+++ b/application/Espo/Core/Authentication/AuthenticationData.php
@@ -0,0 +1,93 @@
+<?php
+/************************************************************************
+ * This file is part of EspoCRM.
+ *
+ * EspoCRM - Open Source CRM application.
+ * Copyright (C) 2014-2021 Yurii Kuznietsov, Taras Machyshyn, Oleksii Avramenko
+ * Website: https://www.espocrm.com
+ *
+ * EspoCRM is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * EspoCRM 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with EspoCRM. If not, see http://www.gnu.org/licenses/.
+ *
+ * The interactive user interfaces in modified source and object code versions
+ * of this program must display Appropriate Legal Notices, as required under
+ * Section 5 of the GNU General Public License version 3.
+ *
+ * In accordance with Section 7(b) of the GNU General Public License version 3,
+ * these Appropriate Legal Notices must retain the display of the "EspoCRM" word.
+ ************************************************************************/
+
+namespace Espo\Core\Authentication;
+
+class AuthenticationData
+{
+    private $username;
+
+    private $password;
+
+    private $method;
+
+    public function __construct(
+        ?string $username = null,
+        ?string $password = null,
+        ?string $method = null
+    ) {
+        $this->username = $username;
+        $this->password = $password;
+        $this->method = $method;
+    }
+
+    public static function create(): self
+    {
+        return new self();
+    }
+
+    public function getUsername(): ?string
+    {
+        return $this->username;
+    }
+
+    public function getPassword(): ?string
+    {
+        return $this->password;
+    }
+
+    public function getMethod(): ?string
+    {
+        return $this->method;
+    }
+
+    public function withUsername(?string $username): self
+    {
+        $obj = clone $this;
+        $obj->username = $username;
+
+        return $obj;
+    }
+
+    public function withPassword(?string $password): self
+    {
+        $obj = clone $this;
+        $obj->password = $password;
+
+        return $obj;
+    }
+
+    public function withMethod(?string $method): self
+    {
+        $obj = clone $this;
+        $obj->method = $method;
+
+        return $obj;
+    }
+}
diff --git a/application/Espo/Core/Authentication/AuthenticationFactory.php b/application/Espo/Core/Authentication/AuthenticationFactory.php
index 28f1e43981..e174c064e6 100644
--- a/application/Espo/Core/Authentication/AuthenticationFactory.php
+++ b/application/Espo/Core/Authentication/AuthenticationFactory.php
@@ -33,7 +33,7 @@ use Espo\Core\InjectableFactory;
 
 class AuthenticationFactory
 {
-    protected $injectableFactory;
+    private $injectableFactory;
 
     public function __construct(InjectableFactory $injectableFactory)
     {
diff --git a/tests/integration/Core/Tester.php b/tests/integration/Core/Tester.php
index 748327fda4..b7340534f4 100644
--- a/tests/integration/Core/Tester.php
+++ b/tests/integration/Core/Tester.php
@@ -30,6 +30,7 @@
 namespace tests\integration\Core;
 
 use Espo\Core\Authentication\Authentication;
+use Espo\Core\Authentication\AuthenticationData;
 
 use Espo\Core\Application;
 use Espo\Core\Portal\Application as PortalApplication;
@@ -212,13 +213,12 @@ class Tester
             if (isset($this->userName) || $this->authenticationMethod) {
                 $this->password = isset($this->password) ? $this->password : $this->defaultUserPassword;
 
-                $auth->login(
-                    $this->userName,
-                    $this->password,
-                    $request,
-                    $response,
-                    $this->authenticationMethod
-                );
+                $authenticationData = AuthenticationData::create()
+                    ->withUsername($this->userName)
+                    ->withPassword($this->password)
+                    ->withMethod($this->authenticationMethod);
+
+                $auth->login($authenticationData, $request, $response);
             }
             else {
                 $this->application->setupSystemUser();
diff --git a/tests/unit/Espo/Core/Authentication/AuthenticationDataTest.php b/tests/unit/Espo/Core/Authentication/AuthenticationDataTest.php
new file mode 100644
index 0000000000..50cec80de8
--- /dev/null
+++ b/tests/unit/Espo/Core/Authentication/AuthenticationDataTest.php
@@ -0,0 +1,59 @@
+<?php
+/************************************************************************
+ * This file is part of EspoCRM.
+ *
+ * EspoCRM - Open Source CRM application.
+ * Copyright (C) 2014-2021 Yurii Kuznietsov, Taras Machyshyn, Oleksii Avramenko
+ * Website: https://www.espocrm.com
+ *
+ * EspoCRM is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * EspoCRM 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with EspoCRM. If not, see http://www.gnu.org/licenses/.
+ *
+ * The interactive user interfaces in modified source and object code versions
+ * of this program must display Appropriate Legal Notices, as required under
+ * Section 5 of the GNU General Public License version 3.
+ *
+ * In accordance with Section 7(b) of the GNU General Public License version 3,
+ * these Appropriate Legal Notices must retain the display of the "EspoCRM" word.
+ ************************************************************************/
+
+namespace tests\unit\Espo\Core\Authentication;
+
+use Espo\Core\Authentication\AuthenticationData;
+
+class AuthenticationDataTest extends \PHPUnit\Framework\TestCase
+{
+    public function testCreate1(): void
+    {
+        $data = AuthenticationData::create()
+            ->withUsername('u')
+            ->withPassword('p')
+            ->withMethod(null);
+
+        $this->assertEquals('u', $data->getUsername());
+        $this->assertEquals('p', $data->getPassword());
+        $this->assertNull($data->getMethod());
+    }
+
+    public function testCreate2(): void
+    {
+        $data = AuthenticationData::create()
+            ->withUsername(null)
+            ->withPassword(null)
+            ->withMethod('m');
+
+        $this->assertNull($data->getUsername());
+        $this->assertNull($data->getPassword());
+        $this->assertEquals('m', $data->getMethod());
+    }
+}