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

Always return an error when confirmation_token is blank #5132

Merged
merged 4 commits into from Sep 4, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
12 changes: 12 additions & 0 deletions lib/devise/models/confirmable.rb
Expand Up @@ -348,7 +348,19 @@ def send_confirmation_instructions(attributes={})
# If the user is already confirmed, create an error for the user
# Options must have the confirmation_token
def confirm_by_token(confirmation_token)
# When the `confirmation_token` parameter is blank, if there are any users with a blank
# `confirmation_token` in the database, the first one would be confirmed here.
# The error is being manually added here to ensure no users are confirmed by mistake.
# This was done in the model for convenience, since validation errors are automatically
# displayed in the view.
if confirmation_token.blank?
confirmable = new
confirmable.errors.add(:confirmation_token, :blank)
return confirmable
end

confirmable = find_first_by_auth_conditions(confirmation_token: confirmation_token)

unless confirmable
confirmation_digest = Devise.token_generator.digest(self, :confirmation_token, confirmation_token)
confirmable = find_or_initialize_with_error_by(:confirmation_token, confirmation_digest)
Expand Down
30 changes: 30 additions & 0 deletions test/integration/confirmable_test.rb
Expand Up @@ -175,6 +175,36 @@ def resend_confirmation
assert_current_url '/users/sign_in'
end

test "should not be able to confirm an email with a blank confirmation token" do
visit_user_confirmation_with_token("")

assert_contain "Confirmation token can't be blank"
end

test "should not be able to confirm an email with a nil confirmation token" do
visit_user_confirmation_with_token(nil)

assert_contain "Confirmation token can't be blank"
end

test "should not be able to confirm user with blank confirmation token" do
user = create_user(confirm: false)
user.update_attribute(:confirmation_token, "")

visit_user_confirmation_with_token("")

assert_contain "Confirmation token can't be blank"
end

test "should not be able to confirm user with nil confirmation token" do
user = create_user(confirm: false)
user.update_attribute(:confirmation_token, nil)

visit_user_confirmation_with_token(nil)

assert_contain "Confirmation token can't be blank"
end

test 'error message is configurable by resource name' do
store_translations :en, devise: {
failure: { user: { unconfirmed: "Not confirmed user" } }
Expand Down
18 changes: 18 additions & 0 deletions test/models/confirmable_test.rb
Expand Up @@ -77,6 +77,24 @@ def setup
assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join
end

test 'should return a new record with errors when a blank token is given and a record exists on the database' do

Choose a reason for hiding this comment

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

I think this no longer needs 'and a record exists on the database'

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 think it's better to keep since that's the behavior we're trying to ensure it doesn't happen anymore.
I know that since the current code validates the parameter before doing anything else, it's unlikely for this to happen, but this test ensures we don't add regressions in the future.

user = create_user(confirmation_token: '')

confirmed_user = User.confirm_by_token('')

refute user.reload.confirmed?
assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join
end

test 'should return a new record with errors when a nil token is given and a record exists on the database' do

Choose a reason for hiding this comment

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

I think this no longer needs 'and a record exists on the database'

user = create_user(confirmation_token: nil)

confirmed_user = User.confirm_by_token(nil)

refute user.reload.confirmed?
assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join
end

test 'should generate errors for a user email if user is already confirmed' do
user = create_user
user.confirmed_at = Time.now
Expand Down