Merge pull request #2890 from BookStackApp/analysis-kabPRy

Apply fixes from StyleCI
This commit is contained in:
Dan Brown 2021-08-21 15:50:16 +01:00 committed by GitHub
commit ac110eb6b2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
31 changed files with 173 additions and 161 deletions

View File

@ -14,6 +14,7 @@ class EmailConfirmationService extends UserTokenService
/**
* Create new confirmation for a user,
* Also removes any existing old ones.
*
* @throws ConfirmationEmailException
*/
public function sendConfirmation(User $user)

View File

@ -13,7 +13,6 @@ use Exception;
class LoginService
{
protected const LAST_LOGIN_ATTEMPTED_SESSION_KEY = 'auth-login-last-attempted';
protected $mfaSession;
@ -30,12 +29,14 @@ class LoginService
* Will start a login of the given user but will prevent if there's
* a reason to (MFA or Unconfirmed Email).
* Returns a boolean to indicate the current login result.
*
* @throws StoppedAuthenticationException
*/
public function login(User $user, string $method, bool $remember = false): void
{
if ($this->awaitingEmailConfirmation($user) || $this->needsMfaVerification($user)) {
$this->setLastLoginAttemptedForUser($user, $method, $remember);
throw new StoppedAuthenticationException($user, $this);
}
@ -55,6 +56,7 @@ class LoginService
/**
* Reattempt a system login after a previous stopped attempt.
*
* @throws Exception
*/
public function reattemptLoginFor(User $user)
@ -75,12 +77,14 @@ class LoginService
public function getLastLoginAttemptUser(): ?User
{
$id = $this->getLastLoginAttemptDetails()['user_id'];
return User::query()->where('id', '=', $id)->first();
}
/**
* Get the details of the last login attempt.
* Checks upon a ttl of about 1 hour since that last attempted login.
*
* @return array{user_id: ?string, method: ?string, remember: bool}
*/
protected function getLastLoginAttemptDetails(): array
@ -94,6 +98,7 @@ class LoginService
$hourAgo = time() - (60 * 60);
if ($time < $hourAgo) {
$this->clearLastLoginAttempted();
return ['user_id' => null, 'method' => null];
}
@ -156,5 +161,4 @@ class LoginService
return $result;
}
}

View File

@ -29,6 +29,7 @@ class BackupCodeService
{
$cleanCode = $this->cleanInputCode($code);
$codes = json_decode($codeSet);
return in_array($cleanCode, $codes);
}
@ -42,6 +43,7 @@ class BackupCodeService
$codes = json_decode($codeSet);
$pos = array_search($cleanCode, $codes, true);
array_splice($codes, $pos, 1);
return json_encode($codes);
}

View File

@ -57,5 +57,4 @@ class MfaSession
{
return 'mfa-verification-passed:' . $user->id;
}
}

View File

@ -38,7 +38,7 @@ class MfaValue extends Model
/** @var MfaValue $mfaVal */
$mfaVal = static::query()->firstOrNew([
'user_id' => $user->id,
'method' => $method
'method' => $method,
]);
$mfaVal->setValue($value);
$mfaVal->save();

View File

@ -51,10 +51,11 @@ class TotpService
public function generateQrCodeSvg(string $url): string
{
$color = Fill::uniformColor(new Rgb(255, 255, 255), new Rgb(32, 110, 167));
return (new Writer(
new ImageRenderer(
new RendererStyle(192, 0, null, null, $color),
new SvgImageBackEnd
new SvgImageBackEnd()
)
))->writeString($url);
}

View File

@ -6,7 +6,6 @@ use Illuminate\Contracts\Validation\Rule;
class TotpValidationRule implements Rule
{
protected $secret;
protected $totpService;

View File

@ -88,6 +88,7 @@ class RegistrationService
session()->flash('sent-email-confirmation', true);
} catch (Exception $e) {
$message = trans('auth.email_confirm_send_error');
throw new UserRegistrationException($message, '/register/confirm');
}
}

View File

@ -2,15 +2,11 @@
namespace BookStack\Auth\Access;
use BookStack\Actions\ActivityType;
use BookStack\Auth\User;
use BookStack\Exceptions\JsonDebugException;
use BookStack\Exceptions\SamlException;
use BookStack\Exceptions\StoppedAuthenticationException;
use BookStack\Exceptions\UserRegistrationException;
use BookStack\Facades\Activity;
use BookStack\Facades\Theme;
use BookStack\Theming\ThemeEvents;
use Exception;
use Illuminate\Support\Str;
use OneLogin\Saml2\Auth;
@ -392,6 +388,7 @@ class Saml2Service extends ExternalAuthService
}
$this->loginService->login($user, 'saml2');
return $user;
}
}

View File

@ -142,6 +142,7 @@ class SocialAuthService
// Simply log the user into the application.
if (!$isLoggedIn && $socialAccount !== null) {
$this->loginService->login($socialAccount->user, $socialAccount);
return redirect()->intended('/');
}

View File

@ -45,6 +45,7 @@ class ResetMfa extends Command
$email = $this->option('email');
if (!$id && !$email) {
$this->error('Either a --id=<number> or --email=<email> option must be provided.');
return 1;
}
@ -57,6 +58,7 @@ class ResetMfa extends Command
if (!$user) {
$this->error("A user where {$field}={$value} could not be found.");
return 1;
}
@ -66,6 +68,7 @@ class ResetMfa extends Command
if ($confirm) {
$user->mfaValues()->delete();
$this->info('User MFA methods have been reset.');
return 0;
}

View File

@ -9,7 +9,6 @@ use Illuminate\Http\Request;
class StoppedAuthenticationException extends \Exception implements Responsable
{
protected $user;
protected $loginService;

View File

@ -28,8 +28,7 @@ class ConfirmEmailController extends Controller
EmailConfirmationService $emailConfirmationService,
LoginService $loginService,
UserRepo $userRepo
)
{
) {
$this->emailConfirmationService = $emailConfirmationService;
$this->loginService = $loginService;
$this->userRepo = $userRepo;
@ -51,6 +50,7 @@ class ConfirmEmailController extends Controller
public function showAwaiting()
{
$user = $this->loginService->getLastLoginAttemptUser();
return view('auth.user-unconfirmed', ['user' => $user]);
}

View File

@ -145,12 +145,15 @@ class LoginController extends Controller
* Attempt to log the user into the application.
*
* @param \Illuminate\Http\Request $request
*
* @return bool
*/
protected function attemptLogin(Request $request)
{
return $this->loginService->attempt(
$this->credentials($request), auth()->getDefaultDriver(), $request->filled('remember')
$this->credentials($request),
auth()->getDefaultDriver(),
$request->filled('remember')
);
}

View File

@ -20,7 +20,7 @@ class MfaBackupCodesController extends Controller
protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-backup-codes';
/**
* Show a view that generates and displays backup codes
* Show a view that generates and displays backup codes.
*/
public function generate(BackupCodeService $codeService)
{
@ -37,6 +37,7 @@ class MfaBackupCodesController extends Controller
/**
* Confirm the setup of backup codes, storing them against the user.
*
* @throws Exception
*/
public function confirm()
@ -52,6 +53,7 @@ class MfaBackupCodesController extends Controller
if (!auth()->check()) {
$this->showSuccessNotification(trans('auth.mfa_setup_login_notification'));
return redirect('/login');
}
@ -60,6 +62,7 @@ class MfaBackupCodesController extends Controller
/**
* Verify the MFA method submission on check.
*
* @throws NotFoundException
* @throws ValidationException
*/
@ -76,8 +79,8 @@ class MfaBackupCodesController extends Controller
if (!$codeService->inputCodeExistsInSet($value, $codes)) {
$fail(trans('validation.backup_codes'));
}
}
]
},
],
]);
$updatedCodes = $codeService->removeInputCodeFromSet($request->get('code'), $codes);

View File

@ -20,6 +20,7 @@ class MfaController extends Controller
->mfaValues()
->get(['id', 'method'])
->groupBy('method');
return view('mfa.setup', [
'userMethods' => $userMethods,
]);
@ -27,6 +28,7 @@ class MfaController extends Controller
/**
* Remove an MFA method for the current user.
*
* @throws \Exception
*/
public function remove(string $method)

View File

@ -43,6 +43,7 @@ class MfaTotpController extends Controller
/**
* Confirm the setup of TOTP and save the auth method secret
* against the current user.
*
* @throws ValidationException
* @throws NotFoundException
*/
@ -54,7 +55,7 @@ class MfaTotpController extends Controller
'required',
'max:12', 'min:4',
new TotpValidationRule($totpSecret),
]
],
]);
MfaValue::upsertWithValue($this->currentOrLastAttemptedUser(), MfaValue::METHOD_TOTP, $totpSecret);
@ -63,6 +64,7 @@ class MfaTotpController extends Controller
if (!auth()->check()) {
$this->showSuccessNotification(trans('auth.mfa_setup_login_notification'));
return redirect('/login');
}
@ -71,6 +73,7 @@ class MfaTotpController extends Controller
/**
* Verify the MFA method submission on check.
*
* @throws NotFoundException
*/
public function verify(Request $request, LoginService $loginService, MfaSession $mfaSession)
@ -83,7 +86,7 @@ class MfaTotpController extends Controller
'required',
'max:12', 'min:4',
new TotpValidationRule($totpSecret),
]
],
]);
$mfaSession->markVerifiedForUser($user);

View File

@ -48,8 +48,7 @@ class RegisterController extends Controller
SocialAuthService $socialAuthService,
RegistrationService $registrationService,
LoginService $loginService
)
{
) {
$this->middleware('guest');
$this->middleware('guard:standard');

View File

@ -27,8 +27,7 @@ class SocialController extends Controller
SocialAuthService $socialAuthService,
RegistrationService $registrationService,
LoginService $loginService
)
{
) {
$this->middleware('guest')->only(['getRegister', 'postRegister']);
$this->socialAuthService = $socialAuthService;
$this->registrationService = $registrationService;

View File

@ -9,7 +9,6 @@ use Illuminate\Http\Request;
class ApiAuthenticate
{
/**
* Handle an incoming request.
*/

View File

@ -16,6 +16,7 @@ class Authenticate
if ($request->ajax()) {
return response('Unauthorized.', 401);
}
return redirect()->guest(url('/login'));
}

View File

@ -8,7 +8,6 @@ use Closure;
class AuthenticatedOrPendingMfa
{
protected $loginService;
protected $mfaSession;
@ -18,12 +17,12 @@ class AuthenticatedOrPendingMfa
$this->mfaSession = $mfaSession;
}
/**
* Handle an incoming request.
*
* @param \Illuminate\Http\Request $request
* @param \Closure $next
*
* @return mixed
*/
public function handle($request, Closure $next)

View File

@ -27,6 +27,7 @@ class FileLoader extends BaseLoader
$themePath = theme_path('lang');
$themeTranslations = $themePath ? $this->loadPath($themePath, $locale, $group) : [];
$originalTranslations = $this->loadPath($this->path, $locale, $group);
return array_merge($originalTranslations, $themeTranslations);
}

View File

@ -223,7 +223,6 @@ Route::group(['middleware' => 'auth'], function () {
Route::get('/roles/{id}', 'RoleController@edit');
Route::put('/roles/{id}', 'RoleController@update');
});
});
// MFA routes

View File

@ -332,7 +332,7 @@ class AuthTest extends BrowserKitTest
$user = $this->getEditor();
$mfaSession = $this->app->make(MfaSession::class);
$mfaSession->markVerifiedForUser($user);;
$mfaSession->markVerifiedForUser($user);
$this->assertTrue($mfaSession->isVerifiedForUser($user));
$this->asAdmin()->visit('/logout');

View File

@ -10,7 +10,6 @@ use Tests\TestCase;
class MfaConfigurationTest extends TestCase
{
public function test_totp_setup()
{
$editor = $this->getEditor();
@ -155,13 +154,12 @@ class MfaConfigurationTest extends TestCase
$resp = $this->actingAs($admin)->get('/mfa/setup');
$resp->assertElementExists('form[action$="/mfa/totp/remove"]');
$resp = $this->delete("/mfa/totp/remove");
$resp->assertRedirect("/mfa/setup");
$resp = $this->delete('/mfa/totp/remove');
$resp->assertRedirect('/mfa/setup');
$resp = $this->followRedirects($resp);
$resp->assertSee('Multi-factor method successfully removed');
$this->assertActivityExists(ActivityType::MFA_REMOVE_METHOD);
$this->assertEquals(0, $admin->mfaValues()->count());
}
}

View File

@ -227,6 +227,7 @@ class MfaVerificationTest extends TestCase
$role = $user->roles->first();
$role->mfa_enforced = true;
$role->save();
try {
$loginService->login($user, 'testing');
} catch (StoppedAuthenticationException $e) {
@ -238,7 +239,6 @@ class MfaVerificationTest extends TestCase
$resp = $this->call($method, $path);
$resp->assertRedirect('/login');
}
}
/**
@ -275,5 +275,4 @@ class MfaVerificationTest extends TestCase
return [$user, $codes, $loginResp];
}
}

View File

@ -58,7 +58,7 @@ class ResetMfaCommandTest extends TestCase
public function test_giving_non_existing_user_shows_error_message()
{
$this->artisan("bookstack:reset-mfa --email=donkeys@example.com")
$this->artisan('bookstack:reset-mfa --email=donkeys@example.com')
->expectsOutput('A user where email=donkeys@example.com could not be found.')
->assertExitCode(1);
}