Merge pull request #6850 from firefly-iii/fix-auth-issue

Expand authentication validation in API.
firefly-iii · Jan 14, 2023 · db0500d · db0500d
2 parents 89d4a34 + 23bbebb
commit db0500d
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 22 deletions.
diff --git a/app/Api/V1/Controllers/System/UserController.php b/app/Api/V1/Controllers/System/UserController.php
@@ -33,6 +33,7 @@
 use FireflyIII\User;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Pagination\LengthAwarePaginator;
+use Illuminate\Support\Facades\Log;
 use League\Fractal\Pagination\IlluminatePaginatorAdapter;
 use League\Fractal\Resource\Collection as FractalCollection;
 use League\Fractal\Resource\Item;
@@ -191,6 +192,13 @@ public function store(UserStoreRequest $request): JsonResponse
     public function update(UserUpdateRequest $request, User $user): JsonResponse
     {
         $data    = $request->getAll();
+
+        // can only update 'blocked' when user is admin.
+        if(!$this->repository->hasRole(auth()->user(), 'owner')) {
+            Log::debug('Quietly drop fields "blocked" and "blocked_code" from request.');
+            unset($data['blocked'], $data['blocked_code']);
+        }
+
         $user    = $this->repository->update($user, $data);
         $manager = $this->getManager();
         // make resource

diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php
@@ -87,7 +87,7 @@ public function handle($request, Closure $next, ...$guards)
      */
     protected function authenticate($request, array $guards)
     {
-        // Log::debug(sprintf('Now in %s', __METHOD__));
+        Log::debug(sprintf('Now in %s', __METHOD__));
         if (0 === count($guards)) {
             // Log::debug('No guards present.');
             // go for default guard:
@@ -98,41 +98,56 @@ protected function authenticate($request, array $guards)
                 /** @noinspection PhpUndefinedMethodInspection */
                 /** @var User $user */
                 $user = $this->auth->authenticate();
-                if (null === $user) {
-                    Log::warning('User is null, throw exception?');
-                }
-                if (null !== $user) {
-                    // Log::debug(get_class($user));
-                    if (1 === (int)$user->blocked) {
-                        $message = (string)trans('firefly.block_account_logout');
-                        if ('email_changed' === $user->blocked_code) {
-                            $message = (string)trans('firefly.email_changed_logout');
-                        }
-                        app('session')->flash('logoutMessage', $message);
-                        $this->auth->logout(); // @phpstan-ignore-line (thinks function is undefined)
-
-                        throw new AuthenticationException('Blocked account.', $guards);
-                    }
-                }
+                $this->validateBlockedUser($user, $guards);
             }
 
             return $this->auth->authenticate(); // @phpstan-ignore-line (thinks function returns void)
         }
         // Log::debug('Guard array is not empty.');
 
         foreach ($guards as $guard) {
-            // Log::debug(sprintf('Now in guard loop, guard is "%s"', $guard));
-            if('api' !== $guard) {
+            Log::debug(sprintf('Now in guard loop, guard is "%s"', $guard));
+            if ('api' !== $guard) {
                 $this->auth->guard($guard)->authenticate();
             }
             $result = $this->auth->guard($guard)->check();
-            // Log::debug(sprintf('Result is %s', var_export($result, true)));
+            Log::debug(sprintf('Result is %s', var_export($result, true)));
             if ($result) {
+                $user = $this->auth->guard($guard)->user();
+                $this->validateBlockedUser($user, $guards);
                 // According to PHPstan the method returns void, but we'll see.
                 return $this->auth->shouldUse($guard); // @phpstan-ignore-line
             }
         }
 
         throw new AuthenticationException('Unauthenticated.', $guards);
     }
+
+    /**
+     * @param  User|null  $user
+     * @param  array  $guards
+     * @return void
+     * @throws AuthenticationException
+     */
+    private function validateBlockedUser(?User $user, array $guards): void
+    {
+        Log::debug(sprintf('Now in %s', __METHOD__));
+        if (null === $user) {
+            Log::warning('User is null, throw exception?');
+        }
+        if (null !== $user) {
+            // Log::debug(get_class($user));
+            if (1 === (int)$user->blocked) {
+                $message = (string)trans('firefly.block_account_logout');
+                if ('email_changed' === $user->blocked_code) {
+                    $message = (string)trans('firefly.email_changed_logout');
+                }
+                Log::warning('User is blocked, cannot use authentication method.');
+                app('session')->flash('logoutMessage', $message);
+                $this->auth->logout(); // @phpstan-ignore-line (thinks function is undefined)
+
+                throw new AuthenticationException('Blocked account.', $guards);
+            }
+        }
+    }
 }
diff --git a/changelog.md b/changelog.md
@@ -6,8 +6,7 @@ Alpha 2
 
 
 - Data import: when you submit a transaction but give the ID of an account of the wrong type, Firefly III will try to create an account of the right type. For example: you submit a deposit but the source account is an expense account: Firefly III will try to create a revenue account instead. 
-
-
+- Security: blocked users can access API, users can unblock themselves using the API.
 
 ## 5.8.0-alpha.1 - 2023-01-08