From 964ae8eee593687f922c873fa7b378bb6e3e39bb Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 22 Jul 2019 10:48:50 +0200 Subject: Change unconfirmed user login behaviour (#11375) Allow access to account settings, 2FA, authorized applications, and account deletions to unconfirmed and pending users, as well as users who had their accounts disabled. Suspended users cannot update their e-mail or password or delete their account. Display account status on account settings page, for example, when an account is frozen, limited, unconfirmed or pending review. After sign up, login users straight away and show a simple page that tells them the status of their account with links to account settings and logout, to reduce onboarding friction and allow users to correct wrongly typed e-mail addresses. Move the final sign-up step of SSO integrations to be the same as above to reduce code duplication. --- spec/controllers/api/base_controller_spec.rb | 42 ++++++++++++++++++++-- spec/controllers/application_controller_spec.rb | 4 +-- .../auth/confirmations_controller_spec.rb | 41 --------------------- .../auth/registrations_controller_spec.rb | 25 ++++++++----- spec/controllers/auth/sessions_controller_spec.rb | 4 +-- .../settings/deletes_controller_spec.rb | 17 +++++++++ spec/features/log_in_spec.rb | 4 +-- spec/models/user_spec.rb | 4 +-- 8 files changed, 82 insertions(+), 59 deletions(-) (limited to 'spec') diff --git a/spec/controllers/api/base_controller_spec.rb b/spec/controllers/api/base_controller_spec.rb index 750ccc8cf..05a42d1c1 100644 --- a/spec/controllers/api/base_controller_spec.rb +++ b/spec/controllers/api/base_controller_spec.rb @@ -15,7 +15,7 @@ describe Api::BaseController do end end - describe 'Forgery protection' do + describe 'forgery protection' do before do routes.draw { post 'success' => 'api/base#success' } end @@ -27,7 +27,45 @@ describe Api::BaseController do end end - describe 'Error handling' do + describe 'non-functional accounts handling' do + let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'read') } + + controller do + before_action :require_user! + end + + before do + routes.draw { post 'success' => 'api/base#success' } + allow(controller).to receive(:doorkeeper_token) { token } + end + + it 'returns http forbidden for unconfirmed accounts' do + user.update(confirmed_at: nil) + post 'success' + expect(response).to have_http_status(403) + end + + it 'returns http forbidden for pending accounts' do + user.update(approved: false) + post 'success' + expect(response).to have_http_status(403) + end + + it 'returns http forbidden for disabled accounts' do + user.update(disabled: true) + post 'success' + expect(response).to have_http_status(403) + end + + it 'returns http forbidden for suspended accounts' do + user.account.suspend! + post 'success' + expect(response).to have_http_status(403) + end + end + + describe 'error handling' do ERRORS_WITH_CODES = { ActiveRecord::RecordInvalid => 422, Mastodon::ValidationError => 422, diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 27946b60f..1811500df 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -187,10 +187,10 @@ describe ApplicationController, type: :controller do expect(response).to have_http_status(200) end - it 'returns http 403 if user who signed in is suspended' do + it 'redirects to account status page' do sign_in(Fabricate(:user, account: Fabricate(:account, suspended: true))) get 'success' - expect(response).to have_http_status(403) + expect(response).to redirect_to(edit_user_registration_path) end end diff --git a/spec/controllers/auth/confirmations_controller_spec.rb b/spec/controllers/auth/confirmations_controller_spec.rb index e9a471fc5..0b6b74ff9 100644 --- a/spec/controllers/auth/confirmations_controller_spec.rb +++ b/spec/controllers/auth/confirmations_controller_spec.rb @@ -50,45 +50,4 @@ describe Auth::ConfirmationsController, type: :controller do end end end - - describe 'GET #finish_signup' do - subject { get :finish_signup } - - let(:user) { Fabricate(:user) } - before do - sign_in user, scope: :user - @request.env['devise.mapping'] = Devise.mappings[:user] - end - - it 'renders finish_signup' do - is_expected.to render_template :finish_signup - expect(assigns(:user)).to have_attributes id: user.id - end - end - - describe 'PATCH #finish_signup' do - subject { patch :finish_signup, params: { user: { email: email } } } - - let(:user) { Fabricate(:user) } - before do - sign_in user, scope: :user - @request.env['devise.mapping'] = Devise.mappings[:user] - end - - context 'when email is valid' do - let(:email) { 'new_' + user.email } - - it 'redirects to root_path' do - is_expected.to redirect_to root_path - end - end - - context 'when email is invalid' do - let(:email) { '' } - - it 'renders finish_signup' do - is_expected.to render_template :finish_signup - end - end - end end diff --git a/spec/controllers/auth/registrations_controller_spec.rb b/spec/controllers/auth/registrations_controller_spec.rb index a4337039e..3e11b34b5 100644 --- a/spec/controllers/auth/registrations_controller_spec.rb +++ b/spec/controllers/auth/registrations_controller_spec.rb @@ -46,6 +46,15 @@ RSpec.describe Auth::RegistrationsController, type: :controller do post :update expect(response).to have_http_status(200) end + + context 'when suspended' do + it 'returns http forbidden' do + request.env["devise.mapping"] = Devise.mappings[:user] + sign_in(Fabricate(:user, account_attributes: { username: 'test', suspended_at: Time.now.utc }), scope: :user) + post :update + expect(response).to have_http_status(403) + end + end end describe 'GET #new' do @@ -94,9 +103,9 @@ RSpec.describe Auth::RegistrationsController, type: :controller do post :create, params: { user: { account_attributes: { username: 'test' }, email: 'test@example.com', password: '12345678', password_confirmation: '12345678' } } end - it 'redirects to login page' do + it 'redirects to setup' do subject - expect(response).to redirect_to new_user_session_path + expect(response).to redirect_to auth_setup_path end it 'creates user' do @@ -120,9 +129,9 @@ RSpec.describe Auth::RegistrationsController, type: :controller do post :create, params: { user: { account_attributes: { username: 'test' }, email: 'test@example.com', password: '12345678', password_confirmation: '12345678' } } end - it 'redirects to login page' do + it 'redirects to setup' do subject - expect(response).to redirect_to new_user_session_path + expect(response).to redirect_to auth_setup_path end it 'creates user' do @@ -148,9 +157,9 @@ RSpec.describe Auth::RegistrationsController, type: :controller do post :create, params: { user: { account_attributes: { username: 'test' }, email: 'test@example.com', password: '12345678', password_confirmation: '12345678', 'invite_code': invite.code } } end - it 'redirects to login page' do + it 'redirects to setup' do subject - expect(response).to redirect_to new_user_session_path + expect(response).to redirect_to auth_setup_path end it 'creates user' do @@ -176,9 +185,9 @@ RSpec.describe Auth::RegistrationsController, type: :controller do post :create, params: { user: { account_attributes: { username: 'test' }, email: 'test@example.com', password: '12345678', password_confirmation: '12345678', 'invite_code': invite.code } } end - it 'redirects to login page' do + it 'redirects to setup' do subject - expect(response).to redirect_to new_user_session_path + expect(response).to redirect_to auth_setup_path end it 'creates user' do diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 71fcc1a6e..87ef4f2bb 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -160,8 +160,8 @@ RSpec.describe Auth::SessionsController, type: :controller do let(:unconfirmed_user) { user.tap { |u| u.update!(confirmed_at: nil) } } let(:accept_language) { 'fr' } - it 'shows a translated login error' do - expect(flash[:alert]).to eq(I18n.t('devise.failure.unconfirmed', locale: accept_language)) + it 'redirects to home' do + expect(response).to redirect_to(root_path) end end diff --git a/spec/controllers/settings/deletes_controller_spec.rb b/spec/controllers/settings/deletes_controller_spec.rb index 35fd64e9b..996872efd 100644 --- a/spec/controllers/settings/deletes_controller_spec.rb +++ b/spec/controllers/settings/deletes_controller_spec.rb @@ -15,6 +15,15 @@ describe Settings::DeletesController do get :show expect(response).to have_http_status(200) end + + context 'when suspended' do + let(:user) { Fabricate(:user, account_attributes: { username: 'alice', suspended_at: Time.now.utc }) } + + it 'returns http forbidden' do + get :show + expect(response).to have_http_status(403) + end + end end context 'when not signed in' do @@ -49,6 +58,14 @@ describe Settings::DeletesController do it 'marks account as suspended' do expect(user.account.reload).to be_suspended end + + context 'when suspended' do + let(:user) { Fabricate(:user, account_attributes: { username: 'alice', suspended_at: Time.now.utc }) } + + it 'returns http forbidden' do + expect(response).to have_http_status(403) + end + end end context 'with incorrect password' do diff --git a/spec/features/log_in_spec.rb b/spec/features/log_in_spec.rb index 53a1f9b12..f6c26cd0f 100644 --- a/spec/features/log_in_spec.rb +++ b/spec/features/log_in_spec.rb @@ -31,12 +31,12 @@ feature "Log in" do context do given(:confirmed_at) { nil } - scenario "A unconfirmed user is not able to log in" do + scenario "A unconfirmed user is able to log in" do fill_in "user_email", with: email fill_in "user_password", with: password click_on I18n.t('auth.login') - is_expected.to have_css(".flash-message", text: failure_message("unconfirmed")) + is_expected.to have_css("div.admin-wrapper") end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 856254ce4..d7c0b5359 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -506,7 +506,7 @@ RSpec.describe User, type: :model do context 'when user is not confirmed' do let(:confirmed_at) { nil } - it { is_expected.to be false } + it { is_expected.to be true } end end @@ -522,7 +522,7 @@ RSpec.describe User, type: :model do context 'when user is not confirmed' do let(:confirmed_at) { nil } - it { is_expected.to be false } + it { is_expected.to be true } end end end -- cgit