Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor registrations#destroy to validate password if required #23636

Merged
merged 3 commits into from
Jul 11, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions dashboard/app/controllers/registrations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,26 @@ def create
end
end

def destroy
# TODO: (madelynkasula) Remove the new_destroy_flow check when the
# ACCOUNT_DELETION_NEW_FLOW experiment is removed.
if params[:new_destroy_flow]
password_required = current_user.encrypted_password.present?
invalid_password = !current_user.valid_password?(params[:password_confirmation])
if password_required && invalid_password
render json: {
error: I18n.t('password.invalid_password_entered')
}, status: :bad_request
return
end
current_user.destroy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not calling super here because Devise's registrations#destroy uses Rails routing and we want to control the response from the front-end (navigate to /home on success, handle errors in the deletion modal on failure).

Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)
return head :no_content
else
super
end
end

def sign_up_params
super.tap do |params|
if params[:user_type] == "teacher"
Expand Down
1 change: 1 addition & 0 deletions dashboard/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ en:
pair_programming: 'I have a partner at my computer'
student_privacy: "Learn more about why you're not seeing your full name <a href='%{student_privacy_blog}' target='_blank'>here</a>."
password:
invalid_password_entered: 'The password you entered is invalid.'
Copy link
Contributor

Choose a reason for hiding this comment

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

This must already exist somewhere. Can we find whatever existing 'bad password' message we have and reuse that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On login, we use a "Invalid email or password" message, which I think would be the closest existing string in dashboard. I could add the error to current_user and return that?

if password_required && invalid_password
  current_user.errors.add :current_password
  render json: {
    errors: current_user.errors.as_json(full_messages: true)
  }, status: :bad_request
  return
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I might prefer that just to be uniform and reuse existing translations. I'm okay if you feel that's more complex than necessary though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, i don't think that's too complex. it might actually make processing the error easier on the front-end. i'll refactor to that

reset_form:
title: 'Forgot your password?'
instructions: "Enter your email address associated with your account below and we'll send you password reset instructions."
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# -*- coding: utf-8 -*-
require 'test_helper'

module RegistrationsControllerTests
#
# Tests over DELETE /users
#
class DestroyTest < ActionDispatch::IntegrationTest
#
# Tests for old destroy flow
#

test "destroys the user" do
user = create :user
sign_in user
assert_destroys(User) do
delete '/users'
end
assert_redirected_to '/'
end

#
# Tests for new destroy flow
#

test "returns bad request if password is required and not provided" do
user = create :user, password: 'password'
sign_in user
assert_does_not_destroy(User) do
delete '/users', params: {new_destroy_flow: true}
end
assert_response :bad_request
end

test "returns bad request if password is required and incorrect" do
user = create :user, password: 'password'
sign_in user
assert_does_not_destroy(User) do
delete '/users', params: {new_destroy_flow: true, password_confirmation: 'notmypassword'}
end
assert_response :bad_request
end

test "destroys the user if password is required and correct" do
user = create :user, password: 'password'
sign_in user
assert_destroys(User) do
delete '/users', params: {new_destroy_flow: true, password_confirmation: 'password'}
end
assert_response :success
end

test "destroys the user if password is not required" do
user = create :user, :with_migrated_google_authentication_option
user.update_attribute(:encrypted_password, nil)
sign_in user
assert_destroys(User) do
delete '/users', params: {new_destroy_flow: true}
end
assert_response :success
end
end
end