Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release 10.1.4 -- resolve lint warnings #283

Merged
merged 5 commits into from
Oct 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions features/fakes/FakeIdBrokerClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Sil\SspBase\Features\fakes;

use Exception;
use InvalidArgumentException;
use Sil\Idp\IdBroker\Client\ServiceException;

Expand Down Expand Up @@ -80,11 +81,11 @@ public function updateUserLastLogin(string $employeeId): array
* Create a new MFA configuration
* @param string $employee_id
* @param string $type
* @param string $label
* @param string|null $label
* @return array|null
* @throws Exception
*/
public function mfaCreate($employee_id, $type, $label = null)
public function mfaCreate(string $employee_id, string $type, string $label = null): ?array
{
if (empty($employee_id)) {
throw new InvalidArgumentException('employee_id is required');
Expand Down Expand Up @@ -125,9 +126,8 @@ public function mfaCreate($employee_id, $type, $label = null)
* Get a list of MFA configurations for given user
* @param string $employee_id
* @return array
* @throws ServiceException
*/
public function mfaList($employee_id)
public function mfaList(string $employee_id): array
{
return [
[
Expand Down
19 changes: 16 additions & 3 deletions modules/expirychecker/src/Auth/Process/ExpiryDate.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class ExpiryDate extends ProcessingFilter
*
* @param array $config Configuration information about this filter.
* @param mixed $reserved For future use.
* @throws Exception
*/
public function __construct(array $config, mixed $reserved)
{
Expand Down Expand Up @@ -65,6 +66,9 @@ public function __construct(array $config, mixed $reserved)
]);
}

/**
* @throws Exception
*/
protected function loadValuesFromConfig(array $config, array $attributeRules): void
{
foreach ($attributeRules as $attribute => $rules) {
Expand Down Expand Up @@ -141,6 +145,9 @@ protected function getExpiryTimestamp(string $expiryDateAttr, array $state): int
return $expiryTimestamp;
}

/**
* @throws Exception
*/
public static function hasSeenSplashPageRecently(): bool
{
$session = Session::getSessionFromRequest();
Expand All @@ -150,6 +157,9 @@ public static function hasSeenSplashPageRecently(): bool
);
}

/**
* @throws Exception
*/
public static function skipSplashPagesFor(int $seconds): void
{
$session = Session::getSessionFromRequest();
Expand All @@ -162,6 +172,9 @@ public static function skipSplashPagesFor(int $seconds): void
$session->save();
}

/**
* @throws Exception
*/
protected function initLogger(array $config): void
{
$loggerClass = $config['loggerClass'] ?? Psr3SamlLogger::class;
Expand Down Expand Up @@ -215,6 +228,7 @@ public function isTimeToWarn(int $expiryTimestamp, int $warnDaysBefore): bool

/**
* @inheritDoc
* @throws Exception
*/
public function process(array &$state): void
{
Expand Down Expand Up @@ -244,7 +258,7 @@ public function process(array &$state): void
]));

if ($this->isExpired($expiryTimestamp)) {
$this->redirectToExpiredPage($state, $accountName, $expiryTimestamp);
$this->redirectToExpiredPage($state, $accountName);
}

// Display a password expiration warning page if it's time to do so.
Expand All @@ -263,9 +277,8 @@ public function process(array &$state): void
*
* @param array $state The state data.
* @param string $accountName The name of the user account.
* @param int $expiryTimestamp When the password expired.
*/
public function redirectToExpiredPage(array &$state, string $accountName, int $expiryTimestamp): void
public function redirectToExpiredPage(array &$state, string $accountName): void
{
assert('is_array($state)');

Expand Down
4 changes: 3 additions & 1 deletion modules/material/src/MaterialController.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace SimpleSAML\Module\material;

use Exception;
use SimpleSAML\Configuration;
use SimpleSAML\XHTML\TemplateControllerInterface;
use Twig\Environment;
Expand All @@ -11,7 +12,7 @@ class MaterialController implements TemplateControllerInterface
/**
* Modify the twig environment after its initialization (e.g. add filters or extensions).
*
* @param \Twig\Environment $twig The current twig environment.
* @param Environment $twig The current twig environment.
* @return void
*/
public function setUpTwig(Environment &$twig): void
Expand All @@ -24,6 +25,7 @@ public function setUpTwig(Environment &$twig): void
*
* @param array $data The current data used by the template.
* @return void
* @throws Exception
*/
public function display(array &$data): void
{
Expand Down
2 changes: 1 addition & 1 deletion modules/mfa/public/prompt-for-mfa.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
if (filter_has_var(INPUT_POST, 'submitMfa')) {
/* @var string|array $mfaSubmission */
$mfaSubmission = filter_input(INPUT_POST, 'mfaSubmission');
if (substr($mfaSubmission, 0, 1) == '{') {
if (str_starts_with($mfaSubmission, '{')) {
briskt marked this conversation as resolved.
Show resolved Hide resolved
$mfaSubmission = json_decode($mfaSubmission, true);
}

Expand Down
9 changes: 7 additions & 2 deletions modules/mfa/public/send-manager-mfa.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@

$logger = LoggerFactory::getAccordingToState($state);

$errorMessage = null;
if (filter_has_var(INPUT_POST, 'send')) {
$errorMessage = Mfa::sendManagerCode($state, $logger);
try {
Mfa::sendManagerCode($state, $logger);
} catch (Exception $e) {
$errorMessage = $e->getMessage();
briskt marked this conversation as resolved.
Show resolved Hide resolved
}
} elseif (filter_has_var(INPUT_POST, 'cancel')) {
$moduleUrl = SimpleSAML\Module::getModuleURL('mfa/prompt-for-mfa.php', [
'StateId' => $stateId,
Expand All @@ -37,7 +42,7 @@

$t = new Template($globalConfig, 'mfa:send-manager-mfa');
$t->data['manager_email'] = $state['managerEmail'];
$t->data['error_message'] = $errorMessage ?? null;
$t->data['error_message'] = $errorMessage;
$t->send();

$logger->info(json_encode([
Expand Down
53 changes: 29 additions & 24 deletions modules/mfa/src/Auth/Process/Mfa.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use SimpleSAML\Module;
use SimpleSAML\Module\mfa\LoggerFactory;
use SimpleSAML\Utils\HTTP;
use Throwable;

/**
* Filter which prompts the user for MFA credentials.
Expand All @@ -25,9 +26,7 @@
*/
class Mfa extends ProcessingFilter
{
const SESSION_TYPE = 'mfa';
const STAGE_SENT_TO_LOW_ON_BACKUP_CODES_NAG = 'mfa:sent_to_low_on_backup_codes_nag';
const STAGE_SENT_TO_MFA_CHANGE_URL = 'mfa:sent_to_mfa_change_url';
const STAGE_SENT_TO_MFA_NEEDED_MESSAGE = 'mfa:sent_to_mfa_needed_message';
const STAGE_SENT_TO_MFA_PROMPT = 'mfa:sent_to_mfa_prompt';
const STAGE_SENT_TO_NEW_BACKUP_CODES_PAGE = 'mfa:sent_to_new_backup_codes_page';
Expand Down Expand Up @@ -79,6 +78,9 @@ public function __construct(array $config, mixed $reserved)
$this->idBrokerClientClass = $config['idBrokerClientClass'] ?? IdBrokerClient::class;
}

/**
* @throws Exception
*/
protected function loadValuesFromConfig(array $config, array $attributes): void
{
foreach ($attributes as $attribute) {
Expand Down Expand Up @@ -159,7 +161,7 @@ protected function getAttributeAllValues(string $attributeName, array $state): ?
* Get an ID Broker client.
*
* @param array $idBrokerConfig
* @return IdBrokerClient
* @return IdBrokerClient|FakeIdBrokerClient
briskt marked this conversation as resolved.
Show resolved Hide resolved
*/
private static function getIdBrokerClient(array $idBrokerConfig): IdBrokerClient|FakeIdBrokerClient
{
Expand Down Expand Up @@ -194,7 +196,7 @@ public static function getMfaOptionById(array $mfaOptions, int $mfaId): array
}

foreach ($mfaOptions as $mfaOption) {
if ((int)$mfaOption['id'] === (int)$mfaId) {
if ((int)$mfaOption['id'] === $mfaId) {
return $mfaOption;
}
}
Expand Down Expand Up @@ -310,16 +312,16 @@ public static function getTemplateFor(string $mfaType): string
* @param array $state
* @return string
*/
protected static function getRelayStateUrl($state)
protected static function getRelayStateUrl(array $state): string
{
if (array_key_exists('saml:RelayState', $state)) {
$samlRelayState = $state['saml:RelayState'];

if (strpos($samlRelayState, "http://") === 0) {
if (str_starts_with($samlRelayState, "http://")) {
return $samlRelayState;
}

if (strpos($samlRelayState, "https://") === 0) {
if (str_starts_with($samlRelayState, "https://")) {
return $samlRelayState;
}
}
Expand Down Expand Up @@ -349,7 +351,7 @@ public static function giveUserNewBackupCodes(array &$state, LoggerInterface $lo
'event' => 'New backup codes result: succeeded',
'employeeId' => $state['employeeId'],
]));
} catch (\Throwable $t) {
} catch (Throwable $t) {
$logger->error(json_encode([
'event' => 'New backup codes result: failed',
'employeeId' => $state['employeeId'],
Expand Down Expand Up @@ -383,7 +385,7 @@ public static function hasMfaOptionsOtherThan(string $excludedMfaType, array $st
{
$mfaOptions = $state['mfaOptions'] ?? [];
foreach ($mfaOptions as $mfaOption) {
if (strval($mfaOption['type']) !== strval($excludedMfaType)) {
if (strval($mfaOption['type']) !== $excludedMfaType) {
return true;
}
}
Expand All @@ -398,12 +400,12 @@ protected function initComposerAutoloader(): void
}
}

protected static function isHeadedToMfaSetupUrl($state, $mfaSetupUrl)
protected static function isHeadedToMfaSetupUrl($state, $mfaSetupUrl): bool
{
if (array_key_exists('saml:RelayState', $state)) {
$currentDestination = self::getRelayStateUrl($state);
if (!empty($currentDestination)) {
return (strpos($currentDestination, $mfaSetupUrl) === 0);
return (str_starts_with($currentDestination, $mfaSetupUrl));
}
}
return false;
Expand All @@ -416,15 +418,16 @@ protected static function isHeadedToMfaSetupUrl($state, $mfaSetupUrl)
*
* @param int $mfaId The ID of the MFA option used.
* @param string $employeeId The Employee ID that this MFA option belongs to.
* @param string $mfaSubmission The value of the MFA submission.
* @param string|array $mfaSubmission The value of the MFA submission.
briskt marked this conversation as resolved.
Show resolved Hide resolved
* @param array $state The array of state information.
* @param bool $rememberMe Whether or not to set remember me cookies
* @param LoggerInterface $logger A PSR-3 compatible logger.
* @param string $mfaType The type of the MFA ('webauthn', 'totp', 'backupcode').
* @param string $rpOrigin The Relying Party Origin (for WebAuthn)
* @return string If validation fails, an error message to show to the
* end user will be returned.
* @throws \Sil\PhpEnv\EnvVarNotFoundException
* @throws EnvVarNotFoundException
* @throws Exception
mtompset marked this conversation as resolved.
Show resolved Hide resolved
*/
public static function validateMfaSubmission(
int $mfaId,
Expand Down Expand Up @@ -458,7 +461,7 @@ public static function validateMfaSubmission(
if ($mfaDataFromBroker === true || count($mfaDataFromBroker) > 0) {
$idBrokerClient->updateUserLastLogin($employeeId);
}
} catch (\Throwable $t) {
} catch (Throwable $t) {
$message = 'Something went wrong while we were trying to do the '
. '2-step verification.';
if ($t instanceof ServiceException) {
Expand Down Expand Up @@ -539,7 +542,7 @@ public static function validateMfaSubmission(
*
* @param array $state
*/
public static function redirectToMfaSetup(array &$state): void
public static function redirectToMfaSetup(array $state): void
briskt marked this conversation as resolved.
Show resolved Hide resolved
{
$mfaSetupUrl = $state['mfaSetupUrl'];

Expand All @@ -566,6 +569,7 @@ public static function redirectToMfaSetup(array &$state): void

/**
* @inheritDoc
* @throws Exception
*/
public function process(array &$state): void
{
Expand Down Expand Up @@ -686,7 +690,7 @@ protected function redirectToMfaPrompt(array &$state, string $employeeId, array
* @param array $mfaOptions
* @param array $state
* @return bool
* @throws EnvVarNotFoundException
* @throws EnvVarNotFoundException|ServiceException
*/
public static function isRememberMeCookieValid(
string $cookieHash,
Expand All @@ -700,7 +704,7 @@ public static function isRememberMeCookieValid(
if ((int)$expireDate > time()) {
$expectedString = self::generateRememberMeCookieString($rememberSecret, $state['employeeId'], $expireDate, $mfaOptions);
$isValid = password_verify($expectedString, $cookieHash);

if ($isValid) {
$idBrokerClient = self::getIdBrokerClient($state['idBrokerConfig']);
$idBrokerClient->updateUserLastLogin($state['employeeId']);
Expand Down Expand Up @@ -734,8 +738,7 @@ public static function generateRememberMeCookieString(
}
}

$string = $rememberSecret . $employeeId . $expireDate . $allMfaIds;
return $string;
return $rememberSecret . $employeeId . $expireDate . $allMfaIds;
}

/**
Expand Down Expand Up @@ -789,7 +792,7 @@ protected static function redirectToOutOfBackupCodesMessage(array &$state, strin
* @param string $employeeId
* @param array $mfaOptions
* @param string $rememberDuration
* @throws \Sil\PhpEnv\EnvVarNotFoundException
* @throws EnvVarNotFoundException
*/
public static function setRememberMeCookies(
string $employeeId,
Expand Down Expand Up @@ -818,8 +821,9 @@ protected static function shouldPromptForMfa(array $mfa): bool
*
* @param array $state The state data.
* @param LoggerInterface $logger A PSR-3 compatible logger.
* @throws Exception
*/
public static function sendManagerCode(array &$state, LoggerInterface $logger): string
public static function sendManagerCode(array &$state, LoggerInterface $logger): void
{
try {
$idBrokerClient = self::getIdBrokerClient($state['idBrokerConfig']);
Expand All @@ -830,13 +834,14 @@ public static function sendManagerCode(array &$state, LoggerInterface $logger):
'event' => 'Manager rescue code sent',
'employeeId' => $state['employeeId'],
]));
} catch (\Throwable $t) {
} catch (Throwable $t) {
$logger->error(json_encode([
'event' => 'Manager rescue code: failed',
'employeeId' => $state['employeeId'],
'error' => $t->getCode() . ': ' . $t->getMessage(),
]));
return 'There was an unexpected error. Please try again or contact tech support for assistance';
throw new Exception('There was an unexpected error. Please try again ' .
'or contact tech support for assistance');
}

$mfaOptions = $state['mfaOptions'];
Expand Down Expand Up @@ -932,7 +937,7 @@ protected static function updateStateWithNewMfaData(array &$state, LoggerInterfa

try {
$newMfaOptions = $idBrokerClient->mfaList($state['employeeId']);
} catch (Exception $e) {
} catch (Exception) {
$log['status'] = 'failed: id-broker exception';
$logger->error(json_encode($log));
return;
Expand Down
3 changes: 1 addition & 2 deletions modules/mfa/src/LoggerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@

use InvalidArgumentException;
use Psr\Log\LoggerInterface;
use Psr\Log\Psr3SamlLogger;
use SimpleSAML\Module\mfa\Assert;
use Sil\Psr3Adapters\Psr3SamlLogger;

class LoggerFactory
{
Expand Down
Loading