From 2123250a5e758c4c6a78a135e7f9f54e724744f5 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 7 Sep 2022 17:54:27 +0200 Subject: [PATCH] Fix multiple issues detected by static analysis --- app/Api/v1/Controllers/SettingController.php | 3 +- app/Api/v1/Resources/GroupResource.php | 5 ++ .../v1/Resources/TwoFAccountReadResource.php | 4 ++ .../v1/Resources/TwoFAccountStoreResource.php | 11 ++++ app/Api/v1/Resources/UserResource.php | 5 ++ app/Console/Commands/CheckDbConnection.php | 2 +- app/Console/Commands/Utils/ResetTrait.php | 12 ++--- app/Http/Controllers/Auth/UserController.php | 2 +- .../Auth/WebAuthnManageController.php | 2 +- app/Http/Middleware/Authenticate.php | 3 +- app/Http/Middleware/KickOutInactiveUser.php | 3 +- app/Http/Middleware/LogUserLastSeen.php | 2 +- app/Models/Group.php | 3 ++ app/Models/TwoFAccount.php | 52 +++++++------------ app/Providers/AuthServiceProvider.php | 1 - app/Providers/RouteServiceProvider.php | 2 +- app/Services/LogoService.php | 6 +-- app/Services/SettingService.php | 6 +-- app/Services/TwoFAccountService.php | 10 ++-- routes/console.php | 1 + 20 files changed, 77 insertions(+), 58 deletions(-) diff --git a/app/Api/v1/Controllers/SettingController.php b/app/Api/v1/Controllers/SettingController.php index cbab7b4e..bc2e94e9 100644 --- a/app/Api/v1/Controllers/SettingController.php +++ b/app/Api/v1/Controllers/SettingController.php @@ -19,14 +19,13 @@ public function index() { $settings = Settings::all(); $settingsResources = collect(); - $settings->each(function ($item, $key) use ($settingsResources) { + $settings->each(function (mixed $item, string $key) use ($settingsResources) { $settingsResources->push([ 'key' => $key, 'value' => $item ]); }); - // return SettingResource::collection($tata); return response()->json($settingsResources->all(), 200); } diff --git a/app/Api/v1/Resources/GroupResource.php b/app/Api/v1/Resources/GroupResource.php index 4d2277f8..c66825fa 100644 --- a/app/Api/v1/Resources/GroupResource.php +++ b/app/Api/v1/Resources/GroupResource.php @@ -4,6 +4,11 @@ use Illuminate\Http\Resources\Json\JsonResource; +/** + * @property mixed $id + * @property string $name + * @property int|null $twofaccounts_count + */ class GroupResource extends JsonResource { /** diff --git a/app/Api/v1/Resources/TwoFAccountReadResource.php b/app/Api/v1/Resources/TwoFAccountReadResource.php index 93d4755a..d50a84a6 100644 --- a/app/Api/v1/Resources/TwoFAccountReadResource.php +++ b/app/Api/v1/Resources/TwoFAccountReadResource.php @@ -2,6 +2,10 @@ namespace App\Api\v1\Resources; +/** + * @property mixed $id + * @property mixed $group_id + */ class TwoFAccountReadResource extends TwoFAccountStoreResource { /** diff --git a/app/Api/v1/Resources/TwoFAccountStoreResource.php b/app/Api/v1/Resources/TwoFAccountStoreResource.php index 835f9fd1..04608681 100644 --- a/app/Api/v1/Resources/TwoFAccountStoreResource.php +++ b/app/Api/v1/Resources/TwoFAccountStoreResource.php @@ -4,6 +4,17 @@ use Illuminate\Http\Resources\Json\JsonResource; +/** + * @property mixed $otp_type + * @property string $account + * @property string $service + * @property string $icon + * @property string $secret + * @property int $digits + * @property string $algorithm + * @property int|null $period + * @property int|null $counter + */ class TwoFAccountStoreResource extends JsonResource { /** diff --git a/app/Api/v1/Resources/UserResource.php b/app/Api/v1/Resources/UserResource.php index dd949408..cbc2938d 100644 --- a/app/Api/v1/Resources/UserResource.php +++ b/app/Api/v1/Resources/UserResource.php @@ -4,6 +4,11 @@ use Illuminate\Http\Resources\Json\JsonResource; +/** + * @property mixed $id + * @property string $name + * @property string $email + */ class UserResource extends JsonResource { /** diff --git a/app/Console/Commands/CheckDbConnection.php b/app/Console/Commands/CheckDbConnection.php index d75bf37b..81829dc6 100644 --- a/app/Console/Commands/CheckDbConnection.php +++ b/app/Console/Commands/CheckDbConnection.php @@ -35,7 +35,7 @@ public function __construct() /** * Execute the console command. * - * @return mixed + * @return int */ public function handle() : int { diff --git a/app/Console/Commands/Utils/ResetTrait.php b/app/Console/Commands/Utils/ResetTrait.php index 1d881fce..60e43b87 100644 --- a/app/Console/Commands/Utils/ResetTrait.php +++ b/app/Console/Commands/Utils/ResetTrait.php @@ -11,7 +11,7 @@ trait ResetTrait /** * Reset icons */ - protected function resetIcons() + protected function resetIcons() : void { $this->deleteIcons(); $this->generateIcons(); @@ -20,7 +20,7 @@ protected function resetIcons() /** * Delete all icons */ - protected function deleteIcons() + protected function deleteIcons() : void { $filesForDelete = \Illuminate\Support\Facades\File::glob('public/icons/*.png'); Storage::delete($filesForDelete); @@ -31,7 +31,7 @@ protected function deleteIcons() /** * Generate icons for seeded accounts */ - protected function generateIcons() + protected function generateIcons() : void { IconGenerator::generateIcon('amazon', IconGenerator::AMAZON); IconGenerator::generateIcon('apple', IconGenerator::APPLE); @@ -49,7 +49,7 @@ protected function generateIcons() /** * Reset DB */ - protected function resetDB(string $seeder) + protected function resetDB(string $seeder) : void { $this->flushDB(); $this->seedDB($seeder); @@ -58,7 +58,7 @@ protected function resetDB(string $seeder) /** * Delete all DB tables */ - protected function flushDB() + protected function flushDB() : void { // Reset the db DB::table('users')->delete(); @@ -78,7 +78,7 @@ protected function flushDB() /** * Seed the DB */ - protected function seedDB(string $seeder) + protected function seedDB(string $seeder) : void { $this->callSilent('db:seed', [ '--class' => $seeder diff --git a/app/Http/Controllers/Auth/UserController.php b/app/Http/Controllers/Auth/UserController.php index d4c16713..e263bdd4 100644 --- a/app/Http/Controllers/Auth/UserController.php +++ b/app/Http/Controllers/Auth/UserController.php @@ -29,7 +29,7 @@ public function update(UserUpdateRequest $request) } if (!config('2fauth.config.isDemoApp') ) { - tap($user)->update([ + $user->update([ 'name' => $validated['name'], 'email' => $validated['email'], ]); diff --git a/app/Http/Controllers/Auth/WebAuthnManageController.php b/app/Http/Controllers/Auth/WebAuthnManageController.php index b81c0071..f01b5679 100644 --- a/app/Http/Controllers/Auth/WebAuthnManageController.php +++ b/app/Http/Controllers/Auth/WebAuthnManageController.php @@ -54,7 +54,7 @@ public function rename(WebauthnRenameRequest $request, string $credential) $validated = $request->validated(); $webAuthnCredential = WebAuthnCredential::where('id', $credential)->firstOrFail(); - $webAuthnCredential->name = $validated['name']; + $webAuthnCredential->name = $validated['name']; // @phpstan-ignore-line $webAuthnCredential->save(); return response()->json([ diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php index a6b5f6df..c104ac79 100644 --- a/app/Http/Middleware/Authenticate.php +++ b/app/Http/Middleware/Authenticate.php @@ -32,7 +32,8 @@ protected function authenticate($request, array $guards) foreach ($guards as $guard) { if ($this->auth->guard($guard)->check()) { - return $this->auth->shouldUse($guard); + $this->auth->shouldUse($guard); + return; } } diff --git a/app/Http/Middleware/KickOutInactiveUser.php b/app/Http/Middleware/KickOutInactiveUser.php index ba7a0737..59c4a1be 100644 --- a/app/Http/Middleware/KickOutInactiveUser.php +++ b/app/Http/Middleware/KickOutInactiveUser.php @@ -16,9 +16,10 @@ class KickOutInactiveUser * * @param \Illuminate\Http\Request $request * @param \Closure $next + * @param string $guards * @return mixed */ - public function handle($request, Closure $next, ...$quards) + public function handle($request, Closure $next, ...$guards) { // We do not track activity of: // - Guest diff --git a/app/Http/Middleware/LogUserLastSeen.php b/app/Http/Middleware/LogUserLastSeen.php index 5fb4e9e8..21d49358 100644 --- a/app/Http/Middleware/LogUserLastSeen.php +++ b/app/Http/Middleware/LogUserLastSeen.php @@ -13,7 +13,7 @@ class LogUserLastSeen * * @param \Illuminate\Http\Request $request * @param \Closure $next - * @param string|null $guards + * @param string $guards * @return mixed */ public function handle($request, Closure $next, ...$guards) diff --git a/app/Models/Group.php b/app/Models/Group.php index 56c7564f..1c41425b 100644 --- a/app/Models/Group.php +++ b/app/Models/Group.php @@ -7,6 +7,9 @@ use Illuminate\Support\Facades\Log; use Illuminate\Database\Eloquent\Factories\HasFactory; +/** + * @property int $twofaccounts_count + */ class Group extends Model { diff --git a/app/Models/TwoFAccount.php b/app/Models/TwoFAccount.php index 6f795210..4d2b8828 100644 --- a/app/Models/TwoFAccount.php +++ b/app/Models/TwoFAccount.php @@ -50,8 +50,6 @@ class TwoFAccount extends Model implements Sortable const DEFAULT_ALGORITHM = self::SHA1; private const IMAGELINK_STORAGE_PATH = 'imagesLink/'; - private const ICON_STORAGE_PATH = 'public/icons/'; - /** * List of OTP types supported by 2FAuth @@ -152,24 +150,6 @@ protected static function boot() // }); } - /** - * Fill the model with an array of attributes. - * - * @param array $attributes - * @return $this - * - * @throws \Illuminate\Database\Eloquent\MassAssignmentException - */ - // public function fill(array $attributes) - // { - // parent::fill($attributes); - - // if ($this->otp_type == self::TOTP && !$this->period) $this->period = self::DEFAULT_PERIOD; - // if ($this->otp_type == self::HOTP && !$this->counter) $this->counter = self::DEFAULT_COUNTER; - - // return $this; - // } - /** * Settings for @spatie/eloquent-sortable package @@ -307,7 +287,7 @@ public function setPeriodAttribute($value) */ public function setCounterAttribute($value) { - $this->attributes['counter'] = is_null($value) && $this->otp_type === self::HOTP ? self::DEFAULT_COUNTER : $value; + $this->attributes['counter'] = blank($value) && $this->otp_type === self::HOTP ? self::DEFAULT_COUNTER : $value; } @@ -316,6 +296,8 @@ public function setCounterAttribute($value) * * @throws InvalidSecretException The secret is not a valid base32 encoded string * @throws UndecipherableException The secret cannot be deciphered + * @throws UnsupportedOtpTypeException The defined OTP type is not supported + * @throws InvalidOtpParameterException One OTP parameter is invalid * @return TotpDto|HotpDto */ public function getOTP() @@ -332,7 +314,15 @@ public function getOTP() $this->initGenerator(); try { - if ( $this->otp_type === self::TOTP || $this->otp_type === self::STEAM_TOTP ) { + if ( $this->otp_type === self::HOTP ) { + + $OtpDto = new HotpDto(); + $OtpDto->otp_type = $this->otp_type; + $counter = $this->generator->getParameter('counter'); + $OtpDto->password = $this->generator->at($counter); + $OtpDto->counter = $this->counter = $counter + 1; + } + else { $OtpDto = new TotpDto(); $OtpDto->otp_type = $this->otp_type; @@ -342,15 +332,6 @@ public function getOTP() : SteamTotp::getAuthCode(base64_encode(Base32::decodeUpper($this->secret))); $OtpDto->period = $this->period; } - else if ( $this->otp_type === self::HOTP ) { - - $OtpDto = new HotpDto(); - $OtpDto->otp_type = $this->otp_type; - $counter = $this->generator->getCounter(); - $OtpDto->password = $this->generator->at($counter); - $OtpDto->counter = $this->counter = $counter + 1; - - } Log::info(sprintf('New OTP generated for TwoFAccount (%s)', $this->id ? 'id:'.$this->id: 'preview')); @@ -475,12 +456,15 @@ private function enforceAsSteam() : void /** * Returns the OTP type of the instanciated OTP generator + * + * @return mixed */ private function getGeneratorOtpType() { return Arr::get($this->generatorClassMap, get_class($this->generator)); } + /** * Returns an otpauth URI built with model attribute values */ @@ -494,6 +478,8 @@ public function getURI() : string /** * Instanciates the OTP generator with model attribute values + * @throws UnsupportedOtpTypeException The defined OTP type is not supported + * @throws InvalidOtpParameterException One OTP parameter is invalid */ private function initGenerator() : void { @@ -604,7 +590,7 @@ private function getDefaultIcon() /** * Returns an acceptable value */ - private function decryptOrReturn($value) + private function decryptOrReturn(mixed $value) : mixed { // Decipher when needed if ( Settings::get('useEncryption') && $value ) @@ -625,7 +611,7 @@ private function decryptOrReturn($value) /** * Encrypt a value */ - private function encryptOrReturn($value) + private function encryptOrReturn(mixed $value) : mixed { // should be replaced by laravel 8 attribute encryption casting return Settings::get('useEncryption') ? Crypt::encryptString($value) : $value; diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index a3cb14af..06f5b637 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -15,7 +15,6 @@ class AuthServiceProvider extends ServiceProvider /** * The policy mappings for the application. * - * @var array */ // protected $policies = [ // 'App\Models\Model' => 'App\Policies\ModelPolicy', diff --git a/app/Providers/RouteServiceProvider.php b/app/Providers/RouteServiceProvider.php index 89383406..2d791d3d 100644 --- a/app/Providers/RouteServiceProvider.php +++ b/app/Providers/RouteServiceProvider.php @@ -41,7 +41,7 @@ public function boot() $this->routes(function () { Route::prefix('api/v1') ->middleware('api.v1') - ->namespace($this->getApiNamespace(1)) + ->namespace($this->getApiNamespace('1')) ->group(base_path('routes/api/v1.php')); // Route::prefix('api/v2') diff --git a/app/Services/LogoService.php b/app/Services/LogoService.php index 9005fc9f..88bbe37c 100644 --- a/app/Services/LogoService.php +++ b/app/Services/LogoService.php @@ -15,12 +15,12 @@ class LogoService protected $tfas; /** - * @var + * @var string */ const TFA_JSON = 'tfa.json'; /** - * @var + * @var string */ const TFA_URL = 'https://2fa.directory/api/v3/tfa.json'; @@ -146,7 +146,7 @@ protected function fetchLogo(string $logoFile) : void /** * Prepare and make some replacement to optimize logo fetching * - * @param string $str + * @param string $domain * @return string Optimized domain name */ protected function cleanDomain(string $domain) : string diff --git a/app/Services/SettingService.php b/app/Services/SettingService.php index 1bbb1a2a..d903b789 100644 --- a/app/Services/SettingService.php +++ b/app/Services/SettingService.php @@ -36,7 +36,7 @@ public function __construct() /** * Get a setting * - * @param string|array $setting A single setting name or an associative array of name:value settings + * @param string $setting A single setting name * @return mixed string|int|boolean|null */ public function get($setting) @@ -135,7 +135,7 @@ private function build() /** * Replaces boolean by a patterned string as appstrack/laravel-options package does not support var type * - * @param mixed $settings + * @param mixed $value * @return string */ private function replaceBoolean(mixed $value) @@ -147,7 +147,7 @@ private function replaceBoolean(mixed $value) /** * Replaces patterned string that represent booleans with real booleans * - * @param mixed $settings + * @param mixed $value * @return mixed */ private function restoreType(mixed $value) diff --git a/app/Services/TwoFAccountService.php b/app/Services/TwoFAccountService.php index 38302521..c556b0c5 100644 --- a/app/Services/TwoFAccountService.php +++ b/app/Services/TwoFAccountService.php @@ -82,6 +82,8 @@ public static function convertMigrationFromGA($migrationUri) : Collection throw new InvalidGoogleAuthMigration(); } + $twofaccounts = array(); + foreach ($otpParameters->getIterator() as $key => $otp_parameters) { try { @@ -123,9 +125,11 @@ public static function convertMigrationFromGA($migrationUri) : Collection /** + * Explode a comma separated list of IDs to an array of IDs * + * @param int|array|string $ids */ - private static function commaSeparatedToArray($ids) + private static function commaSeparatedToArray($ids) : mixed { if(is_string($ids)) { @@ -142,10 +146,10 @@ private static function commaSeparatedToArray($ids) /** * Return the given collection with items marked as Duplicates (using id=-1) if a similar record exists in database * - * @param \Illuminate\Support\Collection + * @param \Illuminate\Support\Collection $twofaccounts * @return \Illuminate\Support\Collection */ - private static function markAsDuplicate($twofaccounts) : Collection + private static function markAsDuplicate(Collection $twofaccounts) : Collection { $storage = TwoFAccount::all(); diff --git a/routes/console.php b/routes/console.php index 0130cb39..e05f4c9a 100644 --- a/routes/console.php +++ b/routes/console.php @@ -1,6 +1,7 @@