Skip to content

Commit

Permalink
FIX: respond with proper error message if user not found
Browse files Browse the repository at this point in the history
  • Loading branch information
arpitjalan committed Nov 21, 2018
1 parent 539f1c6 commit 10cc698
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 9 deletions.
22 changes: 16 additions & 6 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,22 @@ def password_reset
}
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?,

This comment has been minimized.

Copy link
@tgxworld

tgxworld Nov 21, 2018

Contributor

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.

second_factor_required: !valid_second_factor,
backup_enabled: @user.backup_codes_enabled?
}
end
end
end
end
Expand Down
12 changes: 9 additions & 3 deletions spec/requests/users_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,9 @@
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))
Expand All @@ -138,6 +136,14 @@

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
Expand Down

1 comment on commit 10cc698

@arpitjalan
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Please sign in to comment.