Skip to content

Commit 89c234b

Browse files
snipesnipe
authored
Merge pull request #11358 from snipe/fixes/missing_token_lang
Fixed missing password.token string and checked for user existing before attempting to send reset email
2 parents c24052c + 5c30de5 commit 89c234b

File tree

11 files changed

+125
-41
lines changed

11 files changed

+125
-41
lines changed

‎.env.example‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,13 @@ AWS_DEFAULT_REGION=null
146146
# --------------------------------------------
147147
LOGIN_MAX_ATTEMPTS=5
148148
LOGIN_LOCKOUT_DURATION=60
149-
RESET_PASSWORD_LINK_EXPIRES=900
149+
150+
# --------------------------------------------
151+
# OPTIONAL: FORGOTTEN PASSWORD SETTINGS
152+
# --------------------------------------------
153+
RESET_PASSWORD_LINK_EXPIRES=15
154+
PASSWORD_CONFIRM_TIMEOUT=10800
155+
PASSWORD_RESET_MAX_ATTEMPTS_PER_MIN=50
150156

151157
# --------------------------------------------
152158
# OPTIONAL: MISC

‎app/Console/Kernel.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ protected function schedule(Schedule $schedule)
2424
$schedule->command('snipeit:backup')->weekly();
2525
$schedule->command('backup:clean')->daily();
2626
$schedule->command('snipeit:upcoming-audits')->daily();
27+
$schedule->command('auth:clear-resets')->everyFifteenMinutes();
2728
}
2829

2930
/**

‎app/Http/Controllers/Auth/LoginController.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,13 +136,12 @@ private function loginViaSaml(Request $request)
136136

137137
// Better logging
138138
if (!$saml->isEnabled()) {
139-
\Log::warning("SAML page requested, but SAML does not seem to enabled.");
139+
\Log::debug("SAML page requested, but SAML does not seem to enabled.");
140140
} else {
141141
\Log::warning("SAML page requested, but samlData seems empty.");
142142
}
143143
}
144144

145-
\Log::warning("Something else went wrong while trying to login as SAML user");
146145

147146

148147
}

‎app/Http/Controllers/Auth/ResetPasswordController.php‎

Lines changed: 45 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,11 @@
33
namespace App\Http\Controllers\Auth;
44

55
use App\Http\Controllers\Controller;
6-
use App\Http\Requests\SaveUserRequest;
76
use App\Models\Setting;
87
use App\Models\User;
98
use Illuminate\Foundation\Auth\ResetsPasswords;
109
use Illuminate\Http\Request;
11-
use Illuminate\Validation\Rule;
12-
use Illuminate\Validation\Validator;
10+
1311

1412
class ResetPasswordController extends Controller
1513
{
@@ -63,6 +61,14 @@ protected function credentials(Request $request)
6361

6462
public function showResetForm(Request $request, $token = null)
6563
{
64+
65+
$credentials = $request->only('email', 'token');
66+
67+
if (is_null($this->broker()->getUser($credentials))) {
68+
\Log::debug('Password reset form FAILED - this token is not valid.');
69+
return redirect()->route('password.request')->with('error', trans('passwords.token'));
70+
}
71+
6672
return view('auth.passwords.reset')->with(
6773
[
6874
'token' => $token,
@@ -73,38 +79,53 @@ public function showResetForm(Request $request, $token = null)
7379

7480
public function reset(Request $request)
7581
{
82+
83+
$broker = $this->broker();
84+
7685
$messages = [
7786
'password.not_in' => trans('validation.disallow_same_pwd_as_user_fields'),
7887
];
7988

8089
$request->validate($this->rules(), $request->all(), $this->validationErrorMessages());
8190

82-
// Check to see if the user even exists
83-
$user = User::where('username', '=', $request->input('username'))->first();
91+
\Log::debug('Checking if '.$request->input('username').' exists');
92+
// Check to see if the user even exists - we'll treat the response the same to prevent user sniffing
93+
if ($user = User::where('username', '=', $request->input('username'))->where('activated', '1')->whereNotNull('email')->first()) {
94+
\Log::debug($user->username.' exists');
8495

85-
$broker = $this->broker();
86-
if (strpos(Setting::passwordComplexityRulesSaving('store'), 'disallow_same_pwd_as_user_fields') !== false) {
87-
$request->validate(
88-
[
89-
'password' => 'required|notIn:["'.$user->email.'","'.$user->username.'","'.$user->first_name.'","'.$user->last_name.'"',
90-
], $messages);
91-
}
9296

93-
$response = $broker->reset(
94-
$this->credentials($request), function ($user, $password) {
97+
// handle the password validation rules set by the admin settings
98+
if (strpos(Setting::passwordComplexityRulesSaving('store'), 'disallow_same_pwd_as_user_fields') !== false) {
99+
$request->validate(
100+
[
101+
'password' => 'required|notIn:["'.$user->email.'","'.$user->username.'","'.$user->first_name.'","'.$user->last_name.'"',
102+
], $messages);
103+
}
104+
105+
106+
// set the response
107+
$response = $broker->reset(
108+
$this->credentials($request), function ($user, $password) {
95109
$this->resetPassword($user, $password);
110+
});
111+
112+
// Check if the password reset above actually worked
113+
if ($response == \Password::PASSWORD_RESET) {
114+
\Log::debug('Password reset for '.$user->username.' worked');
115+
return redirect()->guest('login')->with('success', trans('passwords.reset'));
96116
}
97-
);
98117

99-
return $response == \Password::PASSWORD_RESET
100-
? $this->sendResetResponse($request, $response)
101-
: $this->sendResetFailedResponse($request, $response);
102-
}
118+
\Log::debug('Password reset for '.$user->username.' FAILED - this user exists but the token is not valid');
119+
return redirect()->back()->withInput($request->only('email'))->with('error', trans('passwords.token'));
120+
121+
}
122+
123+
124+
\Log::debug('Password reset for '.$request->input('username').' FAILED - user does not exist or does not have an email address - but make it look like it succeeded');
125+
return redirect()->guest('login')->with('success', trans('passwords.reset'));
103126

104-
protected function sendResetFailedResponse(Request $request, $response)
105-
{
106-
return redirect()->back()
107-
->withInput(['username'=> $request->input('username')])
108-
->withErrors(['username' => trans($response), 'password' => trans($response)]);
109127
}
128+
129+
130+
110131
}

‎app/Providers/RouteServiceProvider.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,22 @@ protected function mapApiRoutes()
7575
/**
7676
* Configure the rate limiters for the application.
7777
*
78+
* https://laravel.com/docs/8.x/routing#rate-limiting
79+
*
7880
* @return void
7981
*/
8082
protected function configureRateLimiting()
8183
{
84+
85+
// Rate limiter for API calls
8286
RateLimiter::for('api', function (Request $request) {
83-
return Limit::perMinute(60)->by(optional($request->user())->id ?: $request->ip());
87+
return Limit::perMinute(config('app.api_throttle_per_minute'))->by(optional($request->user())->id ?: $request->ip());
8488
});
89+
90+
// Rate limiter for forgotten password requests
91+
RateLimiter::for('forgotten_password', function (Request $request) {
92+
return Limit::perMinute(config('auth.password_reset.max_attempts_per_min'))->by(optional($request->user())->id ?: $request->ip());
93+
});
94+
8595
}
8696
}

‎config/auth.php‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,14 +98,27 @@
9898
'email' => 'auth.emails.password',
9999
'table' => 'password_resets',
100100
'expire' => env('RESET_PASSWORD_LINK_EXPIRES', 900),
101-
'throttle' => 60,
101+
'throttle' => [
102102
'max_attempts' => env('LOGIN_MAX_ATTEMPTS', 5),
103-
104103
'lockout_duration' => env('LOGIN_LOCKOUT_DURATION', 60),
104+
]
105105

106106
],
107107
],
108108

109+
/*
110+
|--------------------------------------------------------------------------
111+
| Resetting Password Requests
112+
|--------------------------------------------------------------------------
113+
| This sets the throttle for forgotten password requests
114+
|
115+
*/
116+
'password_reset' => [
117+
'max_attempts_per_min' => env('PASSWORD_RESET_MAX_ATTEMPTS_PER_MIN', 50),
118+
],
119+
120+
121+
109122
/*
110123
|--------------------------------------------------------------------------
111124
| Password Confirmation Timeout
@@ -117,6 +130,6 @@
117130
|
118131
*/
119132

120-
'password_timeout' => 10800,
133+
'password_timeout' => env('PASSWORD_CONFIRM_TIMEOUT', 10800),
121134

122135
];

‎resources/lang/en/passwords.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
<?php
22

33
return [
4-
'sent' => 'Success: If that email address exists in our system, a password recovery email has been sent.',
5-
'user' => 'No matching active user found with that email.',
4+
'sent' => 'If a matching user with a valid email address exists in our system, a password recovery email has been sent.',
5+
'user' => 'If a matching user with a valid email address exists in our system, a password recovery email has been sent.',
6+
'token' => 'This password reset token is invalid or expired, or does not match the username provided.',
7+
'reset' => 'Your password has been reset!',
68
];

‎resources/lang/en/reminders.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,8 @@
1414
*/
1515

1616
"password" => "Passwords must be six characters and match the confirmation.",
17-
1817
"user" => "Username or email address is incorrect",
19-
20-
"token" => "This password reset token is invalid.",
21-
22-
"sent" => "If a matching email address was found, a password reminder has been sent!",
18+
"token" => 'This password reset token is invalid or expired, or does not match the username provided.',
19+
'sent' => 'If a matching user with a valid email address exists in our system, a password recovery email has been sent.',
2320

2421
);

‎resources/views/layouts/basic.blade.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656

5757
@if (($snipeSettings) && ($snipeSettings->logo!=''))
5858
<center>
59-
<img id="login-logo" src="{{ Storage::disk('public')->url('').e($snipeSettings->logo) }}">
59+
<a href="{{ config('app.url') }}"><img id="login-logo" src="{{ Storage::disk('public')->url('').e($snipeSettings->logo) }}"></a>
6060
</center>
6161
@endif
6262
<!-- Content -->

‎routes/api.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
|
1717
*/
1818

19-
Route::group(['prefix' => 'v1', 'middleware' => ['api', 'throttle:'.config('app.api_throttle_per_minute').',1']], function () {
19+
Route::group(['prefix' => 'v1', 'middleware' => ['api', 'throttle:api']], function () {
2020

2121

2222
Route::get('/', function () {

0 commit comments

Comments
 (0)