reindent / reformat; add type hints for some of the app password stuff; try and make sure someone can only remove their own app password (see revokeAppPassword() )

functions.inc.php

@@ -22,7 +22,8 @@ $min_db_version = 1844;  # update (at least) before a release with the latest fu
  * Check if the user already provided a password but not the second factor
  * @return boolean
  */
-function authentication_mfa_incomplete() {
+function authentication_mfa_incomplete()
+{
     if (isset($_SESSION['sessid'])) {
         if (isset($_SESSION['sessid']['mfa_complete'])) {
             if ($_SESSION['sessid']['mfa_complete'] == false) {

model/Login.php

@@ -200,9 +200,10 @@ class Login
     }
 
     /**
-     * @param string $username
+     * @param string $username - current user
-     * @param string $new_password
+     * @param string $password - password for $username
-     * @param string $old_password
+     * @param string $app_desc - desc. for the app we're adding
+     * @param string $app_pass - password for the app we're adding
      *
      * All passwords need to be plain text; they'll be hashed appropriately
      * as per the configuration in config.inc.php
@@ -210,7 +211,7 @@ class Login
      * @return boolean true on success; false on failure
      * @throws \Exception if invalid user, or db update fails.
      */
-    public function addAppPassword($username, $password, $app_desc, $app_pass): bool
+    public function addAppPassword(string $username, string $password, string $app_desc, string $app_pass): bool
     {
         list(/*NULL*/, $domain) = explode('@', $username);
 
@@ -240,9 +241,13 @@ class Login
         // As PostgeSQL lacks REPLACE we first check and delete any previous rows matching this ip and user
         $exists = db_query_all('SELECT id FROM mailbox_app_password WHERE username = :username AND description = :description',
             ['username' => $username, 'description' => $app_desc,]);
-        if(isset($exists[0]))
+        if (isset($exists[0])) {
-            foreach($exists as $x) db_delete('mailbox_app_password', 'id', $x['id']);
+            foreach ($exists as $x) {
-        $result = db_insert('mailbox_app_password', ['username' => $username, 'description' => $app_desc, 'password_hash' => $app_pass ], Array());
+                db_delete('mailbox_app_password', 'id', $x['id']);
+            }
+        }
+
+        $result = db_insert('mailbox_app_password', ['username' => $username, 'description' => $app_desc, 'password_hash' => $app_pass], []);
 
         if ($result != 1) {
             db_log($domain, 'edit_password', "FAILURE: " . $username);
@@ -292,5 +297,4 @@ class Login
 
         return true;
     }
-
 }

model/TotpPf.php

@@ -1,4 +1,5 @@
 <?php
+
 use OTPHP\TOTP;
 use Endroid\QrCode\Builder\Builder;
 use Endroid\QrCode\Encoding\Encoding;
@@ -40,8 +41,9 @@ class TotpPf
         $totp = TOTP::create();
         $totp->setLabel($username);
         $totp->setIssuer('Postfix Admin');
-        if(Config::read('logo_url'))
+        if (Config::read('logo_url')) {
             $totp->setParameter('image', Config::read('logo_url'));
+        }
         $QR_content = $totp->getProvisioningUri();
         $pTOTP_secret = $totp->getSecret();
         unset($totp);
@@ -57,7 +59,7 @@ class TotpPf
             ->validateResult(false)
             ->build();
         $qr_code = base64_encode($QRresult->getString());
-        return Array($pTOTP_secret, $qr_code);
+        return array($pTOTP_secret, $qr_code);
     }
 
     /**
@@ -67,8 +69,9 @@ class TotpPf
      */
     public function usesTOTP($username): bool
     {
-        if(!(Config::read('totp') == 'YES'))
+        if (!(Config::read('totp') == 'YES')) {
             return false;
+        }
 
         $sql = "SELECT totp_secret FROM {$this->table} WHERE username = :username AND active = :active";
 
@@ -80,8 +83,9 @@ class TotpPf
         ];
 
         $result = db_query_one($sql, $values);
-        if (is_array($result) && isset($result['totp_secret']) && $result['totp_secret'] != '')
+        if (is_array($result) && isset($result['totp_secret']) && $result['totp_secret'] != '') {
             return true;
+        }
         return false;
     }
 
@@ -103,8 +107,9 @@ class TotpPf
         ];
 
         $result = db_query_one($sql, $values);
-        if (!is_array($result) || !isset($result['totp_secret'])) 
+        if (!is_array($result) || !isset($result['totp_secret'])) {
             return false;
+        }
 
         return $this->checkTOTP($result['totp_secret'], $username, $code);
     }
@@ -120,11 +125,12 @@ class TotpPf
     {
         $totp = TOTP::create($secret);
 
-        if ( $totp->now() == $code )
+        if ( $totp->now() == $code ) {
             return true;
-        else
+        } else {
             return false;
         }
+    }
 
     /**
      * @param string $username
@@ -154,7 +160,6 @@ class TotpPf
         } else {
             return '';
         }
-
     }
 
     /**
@@ -243,15 +248,17 @@ class TotpPf
 
         list($local_part, $domain) = explode('@', $username);
 
-        if (!$this->login->login($username, $password)) 
+        if (!$this->login->login($username, $password)) {
             throw new \Exception(Config::Lang('pPassword_password_current_text_error'));
+        }
 
-        if (authentication_has_role('admin'))
+        if (authentication_has_role('admin')) {
             $admin = 1;
-        elseif (authentication_has_role('global-admin'))
+        } elseif (authentication_has_role('global-admin')) {
             $admin = 2;
-        else
+        } else {
             $admin = 0;
+        }
 
         if (empty($Exception_ip)) {
             $error += 1;
@@ -268,22 +275,25 @@ class TotpPf
             flash_error(Config::Lang('pException_user_entire_domain_error'));
         }
 
-        if ( !($admin==2) && $Exception_user == NULL ) {
+        if ( !($admin==2) && $Exception_user == null ) {
             $error += 1;
             flash_error(Config::Lang('pException_user_global_error'));
         }
 
 
-        $values    = Array('ip' => $Exception_ip, 'username' => $Exception_user, 'description' => $Exception_desc);
+        $values    = array('ip' => $Exception_ip, 'username' => $Exception_user, 'description' => $Exception_desc);
 
         if (!$error) {
             // OK to insert/replace.
             // As PostgeSQL lacks REPLACE we first check and delete any previous rows matching this ip and user
             $exists = db_query_all('SELECT id FROM totp_exception_address WHERE ip = :ip AND username = :username',
                 ['ip' => $Exception_ip, 'username' => $Exception_user]);
-            if(isset($exists[0]))
+            if (isset($exists[0])) {
-                foreach($exists as $x) db_delete('totp_exception_address', 'id', $x['id']);
+                foreach ($exists as $x) {
-            $result = db_insert('totp_exception_address', $values, Array());
+                    db_delete('totp_exception_address', 'id', $x['id']);
+                }
+            }
+            $result = db_insert('totp_exception_address', $values, array());
         }
 
         if ($result != 1) {
@@ -333,17 +343,19 @@ class TotpPf
         $exception = $this->getException($Exception_id);
         $error = 0;
 
-        if(strpos($exception['username'],'@'))
+        if (strpos($exception['username'],'@')) {
             list($Exception_local_part, $Exception_domain) = explode('@', $exception['username']);
-        else
+        } else {
             $Exception_domain = $exception['username'];
+        }
 
-        if (authentication_has_role('global-admin'))
+        if (authentication_has_role('global-admin')) {
             $admin = 2;
-        elseif (authentication_has_role('admin'))
+        } elseif (authentication_has_role('admin')) {
             $admin = 1;
-        else
+        } else {
             $admin = 0;
+        }
 
 
         if ( !$admin && strpos($exception['username'],'@') !== false ) {
@@ -351,7 +363,7 @@ class TotpPf
             throw new \Exception(Config::Lang('pException_user_entire_domain_error'));
         }
 
-        if ( !($admin==2) && $exception['username'] == NULL ) {
+        if ( !($admin==2) && $exception['username'] == null ) {
             $error += 1;
             throw new \Exception(Config::Lang('pException_user_global_error'));
         }
@@ -423,7 +435,5 @@ class TotpPf
     {
         return db_query_one("SELECT * FROM totp_exception_address WHERE id=$id");
     }
-
-
 }
 /* vim: set expandtab softtabstop=4 tabstop=4 shiftwidth=4: */

public/upgrade.php

@@ -2324,4 +2324,3 @@ function upgrade_1849_pgsql()
         ");
     }
 }
-

public/users/app-passwords.php

@@ -61,26 +61,29 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") {
         $fPass = $_POST['fPassword_current'];
         $fAppDesc = $_POST['fAppDesc'];
         $fAppPass = $_POST['fAppPass'];
-        addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $admin, $login, $PALANG);
+        addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $login, $PALANG);
-    }
-    if(isset($_POST['fAppId'])){
-        $fAppId     = $_POST['fAppId'];
-        revokeAppPassword($username, $fAppId, $login, $PALANG);
     }
 
+    if (isset($_POST['fAppId']) && is_numeric($_POST['fAppId'])) {
+        $fAppId = (int)$_POST['fAppId'];
+        revokeAppPassword($username, $fAppId, $PALANG);
+    }
 }
 
-if($admin==2)$passwords    = getAllAppPasswords();
+if ($admin == 2) {
-else $passwords            = getAppPasswordsFor($username);
+    $passwords = getAllAppPasswords();
+} else {
+    $passwords = getAppPasswordsFor($username);
+}
 
 foreach ($passwords as $n => $pass) {
-    if($pass['username'] == $username)
+    if ($pass['username'] == $username) {
-        $passwords[$n]['edit'] = 1;
-    if($admin == 2)
         $passwords[$n]['edit'] = 1;
     }
-
+    if ($admin == 2) {
-
+        $passwords[$n]['edit'] = 1;
+    }
+}
 
 
 $smarty->assign('SESSID_USERNAME', $username);
@@ -92,8 +95,17 @@ $smarty->assign('smarty_template', 'app-passwords');
 $smarty->display('index.tpl');
 
 
-
+/**
-function addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $admin, $login, $PALANG){
+ * @param string $username - postfixadmin username
+ * @param string $fPass - postfixadmin password for username
+ * @param string $fAppPass - application password
+ * @param string $fAppDesc - application description
+ * @param Login $login
+ * @param array $PALANG
+ * @return void
+ */
+function addAppPassword(string $username, string $fPass, string $fAppPass, string $fAppDesc, Login $login, array $PALANG)
+{
     try {
         if ($login->addAppPassword($username, $fPass, $fAppDesc, $fAppPass)) {
             flash_info($PALANG['pAppPassAdd_result_success']);
@@ -107,18 +119,38 @@ function addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $admin, $login,
     }
 }
 
-function revokeAppPassword($username, $fAppId, $login, $PALANG){
+/**
-    // No extra password check by design, user might be in a hurry
+ * @todo Why pass Login here? it's not used?
-    $result = db_delete('mailbox_app_password', 'id', $fAppId);
+ */
-    if($result == 1)flash_info($PALANG['pTotp_exceptions_revoked']);
+function revokeAppPassword(string $username, int $fAppId, array $PALANG)
-    else flash_error($PALANG['pPassword_result_error']);
+{
+    // $username should be from $_SESSION and not modifiable by the end user
+    // we don't want someone to be able to delete someone else's app password by guessing an id...
+    $rows = db_query('SELECT id FROM mailbox_app_password WHERE id = :id AND username = :username', ['username' => $username, 'id' => $fAppId]);
+    if (!empty($rows)) {
+        $result = db_delete('mailbox_app_password', 'id', $rows[0]['id']);
+        if ($result == 1) {
+            flash_info($PALANG['pTotp_exceptions_revoked']);
+            return;
+        }
+    }
+    flash_error($PALANG['pPassword_result_error']);
 }
 
-function getAllAppPasswords(){
+/**
+ * @return array
+ */
+function getAllAppPasswords()
+{
     return db_query_all("SELECT * FROM mailbox_app_password");
 }
 
-function getAppPasswordsFor($username){
+/**
+ * @param string $username
+ * @return array
+ */
+function getAppPasswordsFor($username)
+{
     return db_query_all("SELECT * FROM mailbox_app_password WHERE username = :username", ['username' => $username]);
 }
 

public/users/totp-exceptions.php

@@ -80,20 +80,26 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") {
 
 // Generate list of existing exceptions
 
-if($admin==2)$exceptions    = $totppf->getAllExceptions();
+if ($admin==2) {
-else $exceptions            = $totppf->getExceptionsFor($username);
+    $exceptions    = $totppf->getAllExceptions();
+} else {
+    $exceptions            = $totppf->getExceptionsFor($username);
+}
 
 // User can revoke exceptions for own username
 // Admins can revoke exceptions for own domain
 // Global-admin can revoke all exceptions
 foreach ($exceptions as $n => $ex) {
-    if($ex['username'] == $username)
+    if ($ex['username'] == $username) {
         $exceptions[$n]['edit'] = 1;
-    if($admin == 2)
+    }
+    if ($admin == 2) {
         $exceptions[$n]['edit'] = 1;
-    if($admin==1 && $ex['username'] == $domain)
+    }
+    if ($admin==1 && $ex['username'] == $domain) {
         $exceptions[$n]['edit'] = 1;
     }
+}
 
 
 
@@ -108,7 +114,8 @@ $smarty->display('index.tpl');
 
 
 
-function add_exception($username, $fPassword_current, $fException_ip, $fException_desc, $fException_user, $admin, $totppf, $PALANG){
+function add_exception($username, $fPassword_current, $fException_ip, $fException_desc, $fException_user, $admin, $totppf, $PALANG)
+{
     try {
         if ($totppf->addException($username, $fPassword_current, $fException_ip, $fException_user, $fException_desc)) {
             flash_info($PALANG['pTotp_exception_result_success']);
@@ -122,10 +129,13 @@ function add_exception($username, $fPassword_current, $fException_ip, $fExceptio
     }
 }
 
-function revoke_exception($username, $id, $totppf, $PALANG){
+function revoke_exception($username, $id, $totppf, $PALANG)
+{
     // No extra password check by design, user might be in a hurry
     $result = $totppf->deleteException($username, $id);
-    if($result)flash_info($PALANG['pTotp_exceptions_revoked']);
+    if ($result) {
+        flash_info($PALANG['pTotp_exceptions_revoked']);
+    }
 }
 
 

public/users/totp.php

@@ -75,10 +75,10 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") {
     // Does entered code from 2FA-app match the secret
     if ($fTOTP_code == '') {
         $code_checks_out    = true;
-        $fTOTP_secret       = NULL;
+        $fTOTP_secret       = null;
-    }
+    } else {
-    else
         $code_checks_out = $totppf->checkTOTP($fTOTP_secret,  $username, $fTOTP_code);
+    }
 
     // Check that user has successfully generated a TOTP with external device
     if (!$code_checks_out) {
@@ -98,11 +98,13 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") {
             flash_error($e->getMessage());
         }
     }
-   
 }
 
-if( $totppf->usesTOTP($username))   $smarty->assign('show_form', 'hidden');
+if ( $totppf->usesTOTP($username)) {
-else                                $smarty->assign('show_form', 'visible');
+    $smarty->assign('show_form', 'hidden');
+} else {
+    $smarty->assign('show_form', 'visible');
+}
 $smarty->assign('SESSID_USERNAME', $username);
 $smarty->assign('admin', $admin);
 $smarty->assign('pPassword_password_current_text', $pPassword_password_current_text, false);