Merge pull request #13932 from snipe/features/use_allowlist_for_user_logging

Use allowlist for user observer logging

app/Http/Controllers/Auth/LoginController.php

@@ -56,7 +56,6 @@ class LoginController extends Controller
         parent::__construct();
         $this->middleware('guest', ['except' => ['logout', 'postTwoFactorAuth', 'getTwoFactorAuth', 'getTwoFactorEnroll']]);
         Session::put('backUrl', \URL::previous());
-        // $this->ldap = $ldap;
         $this->saml = $saml;
     }
 
@@ -82,7 +81,6 @@ class LoginController extends Controller
         }
 
         if (Setting::getSettings()->login_common_disabled == '1') {
-            \Log::debug('login_common_disabled is set to 1 - return a 403');
             return view('errors.403');
         }
 

app/Http/Controllers/ProfileController.php

@@ -134,6 +134,7 @@ class ProfileController extends Controller
         ];
 
         $validator = \Validator::make($request->all(), $rules);
+
         $validator->after(function ($validator) use ($request, $user) {
             if (! Hash::check($request->input('current_password'), $user->password)) {
                 $validator->errors()->add('current_password', trans('validation.custom.hashed_pass'));
@@ -159,12 +160,14 @@ class ProfileController extends Controller
         });
 
         if (! $validator->fails()) {
-            $user->password = Hash::make($request->input('password'));
-            $user->save();
 
+            $user->password = Hash::make($request->input('password'));
+            // We have to use saveQuietly here because for some reason this method was calling the User Oserver twice :(
+            $user->saveQuietly();
+            
             // Log the user out of other devices
             Auth::logoutOtherDevices($request->input('password'));
-            return redirect()->route('account.password.index')->with('success', 'Password updated!');
+            return redirect()->route('account')->with('success', trans('passwords.password_change'));
 
         }
         return redirect()->back()->withInput()->withErrors($validator);

app/Observers/UserObserver.php

@@ -17,47 +17,78 @@ class UserObserver
     public function updating(User $user)
     {
 
+        // ONLY allow these fields to be stored
+        $allowed_fields = [
+            'email',
+            'activated',
+            'first_name',
+            'last_name',
+            'website',
+            'country',
+            'gravatar',
+            'location_id',
+            'phone',
+            'jobtitle',
+            'manager_id',
+            'employee_num',
+            'username',
+            'notes',
+            'company_id',
+            'ldap_import',
+            'locale',
+            'two_factor_enrolled',
+            'two_factor_optin',
+            'department_id',
+            'address',
+            'address2',
+            'city',
+            'state',
+            'zip',
+            'remote',
+            'start_date',
+            'end_date',
+            'autoassign_licenses',
+            'vip',
+            'password'
+        ];
+        
         $changed = [];
+
         foreach ($user->getRawOriginal() as $key => $value) {
 
-            if ($user->getRawOriginal()[$key] != $user->getAttributes()[$key]) {
+            // Make sure the info is in the allow fields array
+            if (in_array($key, $allowed_fields)) {
 
-                $changed[$key]['old'] = $user->getRawOriginal()[$key];
-                $changed[$key]['new'] = $user->getAttributes()[$key];
+                // Check and see if the value changed
+                if ($user->getRawOriginal()[$key] != $user->getAttributes()[$key]) {
 
-                // Do not store the hashed password in changes
-                if ($key == 'password') {
-                    $changed['password']['old'] = '*************';
-                    $changed['password']['new'] = '*************';
-                }
+                    $changed[$key]['old'] = $user->getRawOriginal()[$key];
+                    $changed[$key]['new'] = $user->getAttributes()[$key];
 
-                // Do not store last login in changes
-                if ($key == 'last_login') {
-                    unset($changed['last_login']);
-                    unset($changed['last_login']);
-                }
+                    // Do not store the hashed password in changes
+                    if ($key == 'password') {
+                        $changed['password']['old'] = '*************';
+                        $changed['password']['new'] = '*************';
+                    }
 
-                if ($key == 'permissions') {
-                    unset($changed['permissions']);
-                    unset($changed['permissions']);
-                }
-
-                if ($key == 'remember_token') {
-                    unset($changed['remember_token']);
-                    unset($changed['remember_token']);
                 }
             }
+
         }
 
-        $logAction = new Actionlog();
-        $logAction->item_type = User::class;
-        $logAction->item_id = $user->id;
-        $logAction->target_type = User::class; // can we instead say $logAction->item = $asset ?
-        $logAction->target_id = $user->id;
-        $logAction->created_at = date('Y-m-d H:i:s');
-        $logAction->user_id = Auth::id();
-        $logAction->log_meta = json_encode($changed);
-        $logAction->logaction('update');
+        if (count($changed) > 0) {
+            $logAction = new Actionlog();
+            $logAction->item_type = User::class;
+            $logAction->item_id = $user->id;
+            $logAction->target_type = User::class; // can we instead say $logAction->item = $asset ?
+            $logAction->target_id = $user->id;
+            $logAction->created_at = date('Y-m-d H:i:s');
+            $logAction->user_id = Auth::id();
+            $logAction->log_meta = json_encode($changed);
+            $logAction->logaction('update');
+        }
+
+
     }
 
     /**

resources/lang/en/passwords.php

@@ -5,4 +5,5 @@ return [
     'user'			=> 'If a matching user with a valid email address exists in our system, a password recovery email has been sent.',
     'token'         => 'This password reset token is invalid or expired, or does not match the username provided.',
     'reset'         => 'Your password has been reset!',
+    'password_change'  => 'Your password has been updated!',
 ];

resources/views/account/view-assets.blade.php

@@ -10,15 +10,17 @@
 @section('content')
 
 @if ($acceptances = \App\Models\CheckoutAcceptance::forUser(Auth::user())->pending()->count())
-  <div class="col-md-12">
-    <div class="alert alert alert-warning fade in">
-      <i class="fas fa-exclamation-triangle faa-pulse animated"></i>
+  <div class="row">
+    <div class="col-md-12">
+      <div class="alert alert alert-warning fade in">
+        <i class="fas fa-exclamation-triangle faa-pulse animated"></i>
 
-      <strong>
-        <a href="{{ route('account.accept') }}" style="color: white;">
-          {{ trans('general.unaccepted_profile_warning', array('count' => $acceptances)) }}
-        </a>
-        </strong>
+        <strong>
+          <a href="{{ route('account.accept') }}" style="color: white;">
+            {{ trans('general.unaccepted_profile_warning', array('count' => $acceptances)) }}
+          </a>
+          </strong>
+      </div>
     </div>
   </div>
 @endif