FIX: respond with proper error message if user not found

FIX: respond with proper error message if user not found

From 10cc698df3297573da30266c7c7e84c6e4b34ed7 Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Wed, 21 Nov 2018 10:47:37 +0530
Subject: [PATCH] FIX: respond with proper error message if user not found


diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 2a5005b..e8d2c9e 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -545,12 +545,22 @@ class UsersController < ApplicationController
             }
           end
         else
-          render json: {
-            is_developer: UsernameCheckerService.is_developer?(@user&.email),
-            admin: @user&.admin?,
-            second_factor_required: !valid_second_factor,
-            backup_enabled: @user&.backup_codes_enabled?
-          }
+          if @error || @user&.errors&.any?
+            render json: {
+              success: false,
+              message: @error,
+              errors: @user&.errors&.to_hash,
+              is_developer: UsernameCheckerService.is_developer?(@user&.email),
+              admin: @user&.admin?
+            }
+          else
+            render json: {
+              is_developer: UsernameCheckerService.is_developer?(@user.email),
+              admin: @user.admin?,
+              second_factor_required: !valid_second_factor,
+              backup_enabled: @user.backup_codes_enabled?
+            }
+          end
         end
       end
     end
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 52d9cc9..f6f1caf 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -122,11 +122,9 @@ describe UsersController do
     end
 
     context 'missing token' do
-      before do
+      it 'disallows login' do
         get "/u/password-reset/#{token}"
-      end
 
-      it 'disallows login' do
         expect(response.status).to eq(200)
 
         expect(CGI.unescapeHTML(response.body))
@@ -138,6 +136,14 @@ describe UsersController do
 
         expect(session[:current_user_id]).to be_blank
       end
+
+      it "responds with proper error message" do
+        get "/u/password-reset/#{token}.json"
+
+        expect(response.status).to eq(200)
+        expect(JSON.parse(response.body)["message"]).to eq(I18n.t('password_reset.no_token'))
+        expect(session[:current_user_id]).to be_blank
+      end
     end
 
     context 'invalid token' do

GitHub

The is_developer and admin keys in the hash have duplicated logic. I would personally extract them into a default_opts hash and merge the new keys in based on the response. In this case, we want to avoid code duplication so that we avoid the case where some one changes one of the hash and forgets about the other in the future.

I’m not sure if we’re using those keys when the request fails since we were not handling the errors at all previously, so I’ve removed the extra (unneeded) keys and just passed the error messages.

1 Like