From 9aa37b32c3307dcb5896e1b768967666a6fdbf65 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 1 Mar 2021 04:59:13 +0100 Subject: [PATCH] Add `details` to error response for `POST /api/v1/accounts` in REST API (#15803) --- app/controllers/api/v1/accounts_controller.rb | 2 ++ app/lib/validation_error_formatter.rb | 32 +++++++++++++++++++ app/validators/blacklisted_email_validator.rb | 4 +-- app/validators/email_mx_validator.rb | 11 ++++--- app/validators/note_length_validator.rb | 2 +- app/validators/unique_username_validator.rb | 2 +- .../unreserved_username_validator.rb | 5 +-- config/locales/activerecord.en.yml | 17 +++++++++- config/locales/en.yml | 4 --- .../auth/sessions_controller_spec.rb | 4 +-- .../blacklisted_email_validator_spec.rb | 4 +-- .../unreserved_username_validator_spec.rb | 8 ++--- 12 files changed, 72 insertions(+), 23 deletions(-) create mode 100644 app/lib/validation_error_formatter.rb diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 953874e1ab..996f1b79bf 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -27,6 +27,8 @@ class Api::V1::AccountsController < Api::BaseController self.response_body = Oj.dump(response.body) self.status = response.status + rescue ActiveRecord::RecordInvalid => e + render json: ValidationErrorFormatter.new(e, :'account.username' => :username, :'invite_request.text' => :reason).as_json, status: :unprocessable_entity end def follow diff --git a/app/lib/validation_error_formatter.rb b/app/lib/validation_error_formatter.rb new file mode 100644 index 0000000000..3f964f739b --- /dev/null +++ b/app/lib/validation_error_formatter.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class ValidationErrorFormatter + def initialize(error, aliases = {}) + @error = error + @aliases = aliases + end + + def as_json + { error: @error.to_s, details: details } + end + + private + + def details + h = {} + + errors.details.each_pair do |attribute_name, attribute_errors| + messages = errors.messages[attribute_name] + + h[@aliases[attribute_name] || attribute_name] = attribute_errors.map.with_index do |error, index| + { error: 'ERR_' + error[:error].to_s.upcase, description: messages[index] } + end + end + + h + end + + def errors + @errors ||= @error.record.errors + end +end diff --git a/app/validators/blacklisted_email_validator.rb b/app/validators/blacklisted_email_validator.rb index 20a1587cc6..1ca73fdcc1 100644 --- a/app/validators/blacklisted_email_validator.rb +++ b/app/validators/blacklisted_email_validator.rb @@ -2,11 +2,11 @@ class BlacklistedEmailValidator < ActiveModel::Validator def validate(user) - return if user.valid_invitation? + return if user.valid_invitation? || user.email.blank? @email = user.email - user.errors.add(:email, I18n.t('users.blocked_email_provider')) if blocked_email? + user.errors.add(:email, :blocked) if blocked_email? end private diff --git a/app/validators/email_mx_validator.rb b/app/validators/email_mx_validator.rb index ef1554494c..9f70a1469a 100644 --- a/app/validators/email_mx_validator.rb +++ b/app/validators/email_mx_validator.rb @@ -4,16 +4,19 @@ require 'resolv' class EmailMxValidator < ActiveModel::Validator def validate(user) + return if user.email.blank? + domain = get_domain(user.email) - if domain.nil? - user.errors.add(:email, I18n.t('users.invalid_email')) + if domain.blank? + user.errors.add(:email, :invalid) else ips, hostnames = resolve_mx(domain) + if ips.empty? - user.errors.add(:email, I18n.t('users.invalid_email_mx')) + user.errors.add(:email, :unreachable) elsif on_blacklist?(hostnames + ips) - user.errors.add(:email, I18n.t('users.blocked_email_provider')) + user.errors.add(:email, :blocked) end end end diff --git a/app/validators/note_length_validator.rb b/app/validators/note_length_validator.rb index 5ff6df6df2..7ea2bb3e53 100644 --- a/app/validators/note_length_validator.rb +++ b/app/validators/note_length_validator.rb @@ -2,7 +2,7 @@ class NoteLengthValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - record.errors.add(attribute, I18n.t('statuses.over_character_limit', max: options[:maximum])) if too_long?(value) + record.errors.add(attribute, :too_long, message: I18n.t('statuses.over_character_limit', max: options[:maximum]), count: options[:maximum]) if too_long?(value) end private diff --git a/app/validators/unique_username_validator.rb b/app/validators/unique_username_validator.rb index f87eb06baf..09c8fadb55 100644 --- a/app/validators/unique_username_validator.rb +++ b/app/validators/unique_username_validator.rb @@ -4,7 +4,7 @@ class UniqueUsernameValidator < ActiveModel::Validator def validate(account) - return if account.username.nil? + return if account.username.blank? normalized_username = account.username.downcase normalized_domain = account.domain&.downcase diff --git a/app/validators/unreserved_username_validator.rb b/app/validators/unreserved_username_validator.rb index 634ceb06e6..974f3ba627 100644 --- a/app/validators/unreserved_username_validator.rb +++ b/app/validators/unreserved_username_validator.rb @@ -3,9 +3,10 @@ class UnreservedUsernameValidator < ActiveModel::Validator def validate(account) @username = account.username - return if @username.nil? - account.errors.add(:username, I18n.t('accounts.reserved_username')) if reserved_username? + return if @username.blank? + + account.errors.add(:username, :reserved) if reserved_username? end private diff --git a/config/locales/activerecord.en.yml b/config/locales/activerecord.en.yml index 8533418cc6..ec8dad1b19 100644 --- a/config/locales/activerecord.en.yml +++ b/config/locales/activerecord.en.yml @@ -5,13 +5,28 @@ en: poll: expires_at: Deadline options: Choices + user: + agreement: Service agreement + email: E-mail address + locale: Locale + password: Password + user/account: + username: Username + user/invite_request: + text: Reason errors: models: account: attributes: username: - invalid: only letters, numbers and underscores + invalid: must contain only letters, numbers and underscores + reserved: is reserved status: attributes: reblog: taken: of status already exists + user: + attributes: + email: + blocked: uses a disallowed e-mail provider + unreachable: does not seem to exist diff --git a/config/locales/en.yml b/config/locales/en.yml index 0c38c5ae11..beb5683469 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -80,7 +80,6 @@ en: other: Toots posts_tab_heading: Toots posts_with_replies: Toots and replies - reserved_username: The username is reserved roles: admin: Admin bot: Bot @@ -1410,11 +1409,8 @@ en: tips: Tips title: Welcome aboard, %{name}! users: - blocked_email_provider: This e-mail provider isn't allowed follow_limit_reached: You cannot follow more than %{limit} people generic_access_help_html: Trouble accessing your account? You may get in touch with %{email} for assistance - invalid_email: The e-mail address is invalid - invalid_email_mx: The e-mail address does not seem to exist invalid_otp_token: Invalid two-factor code invalid_sign_in_token: Invalid security code otp_lost_help_html: If you lost access to both, you may get in touch with %{email} diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index d3a9a11ebb..d03ae51e86 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -69,7 +69,7 @@ RSpec.describe Auth::SessionsController, type: :controller do end it 'shows a login error' do - expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') + expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: I18n.t('activerecord.attributes.user.email')) end it "doesn't log the user in" do @@ -136,7 +136,7 @@ RSpec.describe Auth::SessionsController, type: :controller do end it 'shows a login error' do - expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') + expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: I18n.t('activerecord.attributes.user.email')) end it "doesn't log the user in" do diff --git a/spec/validators/blacklisted_email_validator_spec.rb b/spec/validators/blacklisted_email_validator_spec.rb index f0708dc462..53b355a579 100644 --- a/spec/validators/blacklisted_email_validator_spec.rb +++ b/spec/validators/blacklisted_email_validator_spec.rb @@ -17,7 +17,7 @@ RSpec.describe BlacklistedEmailValidator, type: :validator do let(:blocked_email) { true } it 'calls errors.add' do - expect(errors).to have_received(:add).with(:email, I18n.t('users.blocked_email_provider')) + expect(errors).to have_received(:add).with(:email, :blocked) end end @@ -25,7 +25,7 @@ RSpec.describe BlacklistedEmailValidator, type: :validator do let(:blocked_email) { false } it 'not calls errors.add' do - expect(errors).not_to have_received(:add).with(:email, I18n.t('users.blocked_email_provider')) + expect(errors).not_to have_received(:add).with(:email, :blocked) end end end diff --git a/spec/validators/unreserved_username_validator_spec.rb b/spec/validators/unreserved_username_validator_spec.rb index 0187941b03..cabd6d386b 100644 --- a/spec/validators/unreserved_username_validator_spec.rb +++ b/spec/validators/unreserved_username_validator_spec.rb @@ -13,7 +13,7 @@ RSpec.describe UnreservedUsernameValidator, type: :validator do let(:account) { double(username: username, errors: errors) } let(:errors ) { double(add: nil) } - context '@username.nil?' do + context '@username.blank?' do let(:username) { nil } it 'not calls errors.add' do @@ -21,14 +21,14 @@ RSpec.describe UnreservedUsernameValidator, type: :validator do end end - context '!@username.nil?' do - let(:username) { '' } + context '!@username.blank?' do + let(:username) { 'f' } context 'reserved_username?' do let(:reserved_username) { true } it 'calls erros.add' do - expect(errors).to have_received(:add).with(:username, I18n.t('accounts.reserved_username')) + expect(errors).to have_received(:add).with(:username, :reserved) end end