Merge pull request #171 from vyskocilpavel/dev_fix_script_improvements

fix: resolving comments from PR #165

Author: vyskocilpavel

lib/ChallengeManager.php

@@ -5,6 +5,7 @@
 use SimpleSAML\Configuration;
 use SimpleSAML\Logger;
 use SimpleSAML\Module\perun\databaseCommand\ChallengesDbCmd;
+use SimpleSAML\Error\Exception;
 use Jose\Component\KeyManagement\JWKFactory;
 use Jose\Component\Core\AlgorithmManager;
 use Jose\Component\Signature\JWSBuilder;
@@ -30,6 +31,7 @@ class ChallengeManager
 
     private $challengeDbCmd;
 
+    private $sigAlg;
     private $hashAlg;
     private $challengeLength;
 
@@ -42,45 +44,36 @@ public function __construct()
         $this->challengeDbCmd = new ChallengesDbCmd();
         $config = Configuration::getConfig(self::CONFIG_FILE_NAME);
         $this->hashAlg = $config->getString(self::HASH_ALG, 'sha512');
-        $this->sighAlg = $config->getString(self::SIG_ALG, 'sha512');
+        $this->sigAlg = $config->getString(self::SIG_ALG, 'sha512');
         $this->challengeLength = $config->getInteger(self::CHALLENGE_LENGTH, 32);
         $this->pubKey = $config->getString(self::PUB_KEY);
         $this->privKey = $config->getString(self::PRIV_KEY);
     }
 
     public function generateChallenge($id, $scriptName): string
     {
-        try {
-            $challenge = hash($this->hashAlg, random_bytes($this->challengeLength));
-        } catch (Exception $ex) {
-            http_response_code(500);
-            throw new Exception('ChallengeManager.generateChallenge: Error while generating a challenge');
-        }
+        $challenge = hash($this->hashAlg, random_bytes($this->challengeLength));
 
         if (empty($id) ||
             empty($scriptName) ||
             !$this->challengeDbCmd->insertChallenge($challenge, $id, $scriptName)) {
-            Logger::error(self::LOG_PREFIX . 'Error while creating a challenge');
-            http_response_code(500);
-            throw new Exception('ChallengeManager.generateChallenge: Error while generating a challenge');
+            throw new Exception('ChallengeManager.generateChallenge: Error while storing a challenge to DB.');
         }
         return $challenge;
     }
 
-    public function generateToken($id, $scriptName, $data)
+    public function generateToken($id, $scriptName, $data): string
     {
-
-        $challenge = $this->generateChallenge($id, $scriptName);
-
-        if (empty($challenge)) {
-            Logger::error('Retrieving the challenge was not successful.');
-            return;
+        try {
+            $challenge = $this->generateChallenge($id, $scriptName);
+        } catch (\Exception $ex) {
+            throw new Exception('ChallengeManager.generateToken: Error while generating a challenge');
         }
 
         $jwk = JWKFactory::createFromKeyFile($this->privKey);
         $algorithmManager = new AlgorithmManager(
             [
-                self::getAlgorithm('Signature\\Algorithm', $this->sighAlg)
+                self::getAlgorithm('Signature\\Algorithm', $this->sigAlg)
             ]
         );
         $jwsBuilder = new JWSBuilder($algorithmManager);
@@ -90,13 +83,14 @@ public function generateToken($id, $scriptName, $data)
             'exp' => time() + 300,
             'challenge' => $challenge,
             'id' => $id,
+            'scriptName' => $scriptName,
             'data' => $data
         ]);
 
         $jws = $jwsBuilder
             ->create()
             ->withPayload($payload)
-            ->addSignature($jwk, ['alg' => $this->sighAlg])
+            ->addSignature($jwk, ['alg' => $this->sigAlg])
             ->build();
 
         $serializer = new CompactSerializer();
@@ -109,7 +103,7 @@ public function decodeToken($token)
     {
         $algorithmManager = new AlgorithmManager(
             [
-                self::getAlgorithm('Signature\\Algorithm', $this->sighAlg)
+                self::getAlgorithm('Signature\\Algorithm', $this->sigAlg)
             ]
         );
         $jwsVerifier = new JWSVerifier($algorithmManager);
@@ -119,7 +113,7 @@ public function decodeToken($token)
         $jws = $serializerManager->unserialize($token);
 
         $headerCheckerManager = new HeaderCheckerManager(
-            [new AlgorithmChecker([$this->sighAlg])],
+            [new AlgorithmChecker([$this->sigAlg])],
             [new JWSTokenSupport()]
         );
 
@@ -128,9 +122,7 @@ public function decodeToken($token)
         $isVerified = $jwsVerifier->verifyWithKey($jws, $jwk, 0);
 
         if (!$isVerified) {
-            Logger::error('Perun.updateUes: The token signature is invalid!');
-            http_response_code(401);
-            exit;
+            throw new Exception('ChallengeManager.decodeToken: The token signature is invalid!');
         }
 
         $claimCheckerManager = new ClaimCheckerManager(
@@ -146,10 +138,11 @@ public function decodeToken($token)
 
         $challenge = $claims['challenge'];
         $id = $claims['id'];
+        $scriptName = $claims['scriptName'];
 
         $challengeManager = new ChallengeManager();
 
-        $challengeDb = $challengeManager->readChallengeFromDb($id);
+        $challengeDb = $challengeManager->readChallengeFromDb($id, $scriptName);
 
         $checkAccessSucceeded = self::checkAccess($challenge, $challengeDb);
         $challengeSuccessfullyDeleted = $challengeManager->deleteChallengeFromDb($id);
@@ -161,32 +154,23 @@ public function decodeToken($token)
         return $claims;
     }
 
-    private function readChallengeFromDb($id)
+    private function readChallengeFromDb($id, $scriptName)
     {
-        if (empty($id)) {
-            http_response_code(400);
+        if (empty($id) || empty($scriptName)) {
             return null;
         }
 
-        $result = $this->challengeDbCmd->readChallenge($id);
-
-        if ($result === null) {
-            http_response_code(500);
-        }
-
-        return $result;
+        return $this->challengeDbCmd->readChallenge($id, $scriptName);
     }
 
     private static function checkAccess($challenge, $challengeDb): bool
     {
         if (empty($challenge) || empty($challengeDb)) {
-            http_response_code(400);
             return false;
         }
 
         if (!hash_equals($challengeDb, $challenge)) {
             Logger::error(self::LOG_PREFIX . 'Hashes are not equal.');
-            http_response_code(401);
             return false;
         }
 
@@ -196,13 +180,11 @@ private static function checkAccess($challenge, $challengeDb): bool
     private function deleteChallengeFromDb($id): bool
     {
         if (empty($id)) {
-            http_response_code(400);
             return false;
         }
 
         if (!$this->challengeDbCmd->deleteChallenge($id)) {
             Logger::error(self::LOG_PREFIX . 'Error while deleting challenge from the database.');
-            http_response_code(500);
             return false;
         }
 

lib/databaseCommand/ChallengesDbCmd.php

@@ -2,16 +2,18 @@
 
 namespace SimpleSAML\Module\perun\databaseCommand;
 
+use SimpleSAML\Logger;
+
 /**
  * @author Dominik Baranek <[email protected]>
  */
 class ChallengesDbCmd extends DatabaseCommand
 {
-    const CHALLENGES_TABLE_NAME = 'scriptChallenges';
-    const ID_COLUMN = 'id';
-    const CHALLENGE_COLUMN = 'challenge';
-    const SCRIPT_COLUMN = 'script';
-    const DATE_COLUMN = 'date';
+    private const CHALLENGES_TABLE_NAME = 'scriptChallenges';
+    private const ID_COLUMN = 'id';
+    private const CHALLENGE_COLUMN = 'challenge';
+    private const SCRIPT_COLUMN = 'script';
+    private const DATE_COLUMN = 'date';
 
     public function __construct()
     {
@@ -33,13 +35,14 @@ public function insertChallenge($challenge, $id, $scriptName): bool
         return $this->write($query, $params);
     }
 
-    public function readChallenge($id)
+    public function readChallenge($id, $scriptName)
     {
         $query = 'SELECT challenge FROM ' . self::CHALLENGES_TABLE_NAME . ' WHERE ' .
-            self::ID_COLUMN . ' = :' . self::ID_COLUMN;
+            self::ID_COLUMN . ' = :' . self::ID_COLUMN . ' AND ' . self::SCRIPT_COLUMN  . ' = :' . self::SCRIPT_COLUMN ;
 
         $params = [
-            self::ID_COLUMN => $id
+            self::ID_COLUMN => $id,
+            self::SCRIPT_COLUMN => $scriptName
         ];
 
         return $this->read($query, $params)->fetchColumn();