From 4e4b3a0c8e69a724e229f028896ce774ef26df3b Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Fri, 11 Sep 2020 20:56:35 +0200
Subject: [PATCH] Refactor settings controllers (#14767)
- Disallow suspended accounts from revoking sessions and apps
- Allow suspended accounts to access exports
---
.../concerns/export_controller_concern.rb | 5 ----
.../authorized_applications_controller.rb | 5 ++++
.../settings/aliases_controller.rb | 4 +--
.../settings/applications_controller.rb | 3 --
app/controllers/settings/base_controller.rb | 7 +++++
.../settings/deletes_controller.rb | 9 ++----
.../exports/blocked_accounts_controller.rb | 2 +-
.../exports/blocked_domains_controller.rb | 2 +-
.../exports/following_accounts_controller.rb | 2 +-
.../settings/exports/lists_controller.rb | 2 +-
.../exports/muted_accounts_controller.rb | 2 +-
.../settings/exports_controller.rb | 11 --------
.../settings/featured_tags_controller.rb | 3 --
.../settings/identity_proofs_controller.rb | 3 --
.../settings/imports_controller.rb | 3 --
.../migration/redirects_controller.rb | 11 ++------
.../settings/migrations_controller.rb | 9 +-----
.../settings/pictures_controller.rb | 1 -
.../settings/preferences_controller.rb | 4 ---
.../settings/profiles_controller.rb | 3 --
.../settings/sessions_controller.rb | 6 ++--
.../confirmations_controller.rb | 5 +---
.../otp_authentication_controller.rb | 5 +---
.../recovery_codes_controller.rb | 7 ++---
.../webauthn_credentials_controller.rb | 3 +-
...actor_authentication_methods_controller.rb | 5 +---
.../auth/registrations/_sessions.html.haml | 2 +-
app/views/auth/registrations/edit.html.haml | 27 +++++++++---------
.../authorized_applications/index.html.haml | 2 +-
config/navigation.rb | 2 +-
.../settings/deletes_controller_spec.rb | 28 +++++++++----------
31 files changed, 65 insertions(+), 118 deletions(-)
diff --git a/app/controllers/concerns/export_controller_concern.rb b/app/controllers/concerns/export_controller_concern.rb
index bfe990c827..24cfc7a012 100644
--- a/app/controllers/concerns/export_controller_concern.rb
+++ b/app/controllers/concerns/export_controller_concern.rb
@@ -5,7 +5,6 @@ module ExportControllerConcern
included do
before_action :authenticate_user!
- before_action :require_not_suspended!
before_action :load_export
skip_before_action :require_functional!
@@ -30,8 +29,4 @@ module ExportControllerConcern
def export_filename
"#{controller_name}.csv"
end
-
- def require_not_suspended!
- forbidden if current_account.suspended?
- end
end
diff --git a/app/controllers/oauth/authorized_applications_controller.rb b/app/controllers/oauth/authorized_applications_controller.rb
index fb8389034b..45151cdd77 100644
--- a/app/controllers/oauth/authorized_applications_controller.rb
+++ b/app/controllers/oauth/authorized_applications_controller.rb
@@ -5,6 +5,7 @@ class Oauth::AuthorizedApplicationsController < Doorkeeper::AuthorizedApplicatio
before_action :store_current_location
before_action :authenticate_resource_owner!
+ before_action :require_not_suspended!, only: :destroy
before_action :set_body_classes
skip_before_action :require_functional!
@@ -25,4 +26,8 @@ class Oauth::AuthorizedApplicationsController < Doorkeeper::AuthorizedApplicatio
def store_current_location
store_location_for(:user, request.url)
end
+
+ def require_not_suspended!
+ forbidden if current_account.suspended?
+ end
end
diff --git a/app/controllers/settings/aliases_controller.rb b/app/controllers/settings/aliases_controller.rb
index b7c9a409d1..a421b8ede3 100644
--- a/app/controllers/settings/aliases_controller.rb
+++ b/app/controllers/settings/aliases_controller.rb
@@ -1,9 +1,9 @@
# frozen_string_literal: true
class Settings::AliasesController < Settings::BaseController
- layout 'admin'
+ skip_before_action :require_functional!
- before_action :authenticate_user!
+ before_action :require_not_suspended!
before_action :set_aliases, except: :destroy
before_action :set_alias, only: :destroy
diff --git a/app/controllers/settings/applications_controller.rb b/app/controllers/settings/applications_controller.rb
index ed3f82a8e0..d3ac268d86 100644
--- a/app/controllers/settings/applications_controller.rb
+++ b/app/controllers/settings/applications_controller.rb
@@ -1,9 +1,6 @@
# frozen_string_literal: true
class Settings::ApplicationsController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
before_action :set_application, only: [:show, :update, :destroy, :regenerate]
before_action :prepare_scopes, only: [:create, :update]
diff --git a/app/controllers/settings/base_controller.rb b/app/controllers/settings/base_controller.rb
index 3c404cfff2..8311538a56 100644
--- a/app/controllers/settings/base_controller.rb
+++ b/app/controllers/settings/base_controller.rb
@@ -1,6 +1,9 @@
# frozen_string_literal: true
class Settings::BaseController < ApplicationController
+ layout 'admin'
+
+ before_action :authenticate_user!
before_action :set_body_classes
before_action :set_cache_headers
@@ -13,4 +16,8 @@ class Settings::BaseController < ApplicationController
def set_cache_headers
response.headers['Cache-Control'] = 'no-cache, no-store, max-age=0, must-revalidate'
end
+
+ def require_not_suspended!
+ forbidden if current_account.suspended?
+ end
end
diff --git a/app/controllers/settings/deletes_controller.rb b/app/controllers/settings/deletes_controller.rb
index 15a59c999d..7d4844e60f 100644
--- a/app/controllers/settings/deletes_controller.rb
+++ b/app/controllers/settings/deletes_controller.rb
@@ -1,14 +1,11 @@
# frozen_string_literal: true
class Settings::DeletesController < Settings::BaseController
- layout 'admin'
-
- before_action :check_enabled_deletion
- before_action :authenticate_user!
- before_action :require_not_suspended!
-
skip_before_action :require_functional!
+ before_action :require_not_suspended!
+ before_action :check_enabled_deletion
+
def show
@confirmation = Form::DeleteConfirmation.new
end
diff --git a/app/controllers/settings/exports/blocked_accounts_controller.rb b/app/controllers/settings/exports/blocked_accounts_controller.rb
index 2092104e01..2190caa361 100644
--- a/app/controllers/settings/exports/blocked_accounts_controller.rb
+++ b/app/controllers/settings/exports/blocked_accounts_controller.rb
@@ -2,7 +2,7 @@
module Settings
module Exports
- class BlockedAccountsController < ApplicationController
+ class BlockedAccountsController < BaseController
include ExportControllerConcern
def index
diff --git a/app/controllers/settings/exports/blocked_domains_controller.rb b/app/controllers/settings/exports/blocked_domains_controller.rb
index 6676ce3401..bee4b2431e 100644
--- a/app/controllers/settings/exports/blocked_domains_controller.rb
+++ b/app/controllers/settings/exports/blocked_domains_controller.rb
@@ -2,7 +2,7 @@
module Settings
module Exports
- class BlockedDomainsController < ApplicationController
+ class BlockedDomainsController < BaseController
include ExportControllerConcern
def index
diff --git a/app/controllers/settings/exports/following_accounts_controller.rb b/app/controllers/settings/exports/following_accounts_controller.rb
index 74281ddca2..acefcb15da 100644
--- a/app/controllers/settings/exports/following_accounts_controller.rb
+++ b/app/controllers/settings/exports/following_accounts_controller.rb
@@ -2,7 +2,7 @@
module Settings
module Exports
- class FollowingAccountsController < ApplicationController
+ class FollowingAccountsController < BaseController
include ExportControllerConcern
def index
diff --git a/app/controllers/settings/exports/lists_controller.rb b/app/controllers/settings/exports/lists_controller.rb
index cf5a9de44b..bc65f56a0e 100644
--- a/app/controllers/settings/exports/lists_controller.rb
+++ b/app/controllers/settings/exports/lists_controller.rb
@@ -2,7 +2,7 @@
module Settings
module Exports
- class ListsController < ApplicationController
+ class ListsController < BaseController
include ExportControllerConcern
def index
diff --git a/app/controllers/settings/exports/muted_accounts_controller.rb b/app/controllers/settings/exports/muted_accounts_controller.rb
index e511619ca6..50b7bf1f79 100644
--- a/app/controllers/settings/exports/muted_accounts_controller.rb
+++ b/app/controllers/settings/exports/muted_accounts_controller.rb
@@ -2,7 +2,7 @@
module Settings
module Exports
- class MutedAccountsController < ApplicationController
+ class MutedAccountsController < BaseController
include ExportControllerConcern
def index
diff --git a/app/controllers/settings/exports_controller.rb b/app/controllers/settings/exports_controller.rb
index 0e93d07a9b..30138d29ed 100644
--- a/app/controllers/settings/exports_controller.rb
+++ b/app/controllers/settings/exports_controller.rb
@@ -3,11 +3,6 @@
class Settings::ExportsController < Settings::BaseController
include Authorization
- layout 'admin'
-
- before_action :authenticate_user!
- before_action :require_not_suspended!
-
skip_before_action :require_functional!
def show
@@ -16,8 +11,6 @@ class Settings::ExportsController < Settings::BaseController
end
def create
- raise Mastodon::NotPermittedError unless user_signed_in?
-
backup = nil
RedisLock.acquire(lock_options) do |lock|
@@ -37,8 +30,4 @@ class Settings::ExportsController < Settings::BaseController
def lock_options
{ redis: Redis.current, key: "backup:#{current_user.id}" }
end
-
- def require_not_suspended!
- forbidden if current_account.suspended?
- end
end
diff --git a/app/controllers/settings/featured_tags_controller.rb b/app/controllers/settings/featured_tags_controller.rb
index e9861da56c..e805527d07 100644
--- a/app/controllers/settings/featured_tags_controller.rb
+++ b/app/controllers/settings/featured_tags_controller.rb
@@ -1,9 +1,6 @@
# frozen_string_literal: true
class Settings::FeaturedTagsController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
before_action :set_featured_tags, only: :index
before_action :set_featured_tag, except: [:index, :create]
before_action :set_recently_used_tags, only: :index
diff --git a/app/controllers/settings/identity_proofs_controller.rb b/app/controllers/settings/identity_proofs_controller.rb
index 3a90b7c4df..bf2899da66 100644
--- a/app/controllers/settings/identity_proofs_controller.rb
+++ b/app/controllers/settings/identity_proofs_controller.rb
@@ -1,9 +1,6 @@
# frozen_string_literal: true
class Settings::IdentityProofsController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
before_action :check_required_params, only: :new
def index
diff --git a/app/controllers/settings/imports_controller.rb b/app/controllers/settings/imports_controller.rb
index 7b8c4ae235..d4516526ee 100644
--- a/app/controllers/settings/imports_controller.rb
+++ b/app/controllers/settings/imports_controller.rb
@@ -1,9 +1,6 @@
# frozen_string_literal: true
class Settings::ImportsController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
before_action :set_account
def show
diff --git a/app/controllers/settings/migration/redirects_controller.rb b/app/controllers/settings/migration/redirects_controller.rb
index 97193ade02..6d469f3842 100644
--- a/app/controllers/settings/migration/redirects_controller.rb
+++ b/app/controllers/settings/migration/redirects_controller.rb
@@ -1,13 +1,10 @@
# frozen_string_literal: true
class Settings::Migration::RedirectsController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
- before_action :require_not_suspended!
-
skip_before_action :require_functional!
+ before_action :require_not_suspended!
+
def new
@redirect = Form::Redirect.new
end
@@ -38,8 +35,4 @@ class Settings::Migration::RedirectsController < Settings::BaseController
def resource_params
params.require(:form_redirect).permit(:acct, :current_password, :current_username)
end
-
- def require_not_suspended!
- forbidden if current_account.suspended?
- end
end
diff --git a/app/controllers/settings/migrations_controller.rb b/app/controllers/settings/migrations_controller.rb
index 68304bb513..62603aba81 100644
--- a/app/controllers/settings/migrations_controller.rb
+++ b/app/controllers/settings/migrations_controller.rb
@@ -1,15 +1,12 @@
# frozen_string_literal: true
class Settings::MigrationsController < Settings::BaseController
- layout 'admin'
+ skip_before_action :require_functional!
- before_action :authenticate_user!
before_action :require_not_suspended!
before_action :set_migrations
before_action :set_cooldown
- skip_before_action :require_functional!
-
def show
@migration = current_account.migrations.build
end
@@ -44,8 +41,4 @@ class Settings::MigrationsController < Settings::BaseController
def on_cooldown?
@cooldown.present?
end
-
- def require_not_suspended!
- forbidden if current_account.suspended?
- end
end
diff --git a/app/controllers/settings/pictures_controller.rb b/app/controllers/settings/pictures_controller.rb
index df2a6eed3e..28df65f8ff 100644
--- a/app/controllers/settings/pictures_controller.rb
+++ b/app/controllers/settings/pictures_controller.rb
@@ -2,7 +2,6 @@
module Settings
class PicturesController < BaseController
- before_action :authenticate_user!
before_action :set_account
before_action :set_picture
diff --git a/app/controllers/settings/preferences_controller.rb b/app/controllers/settings/preferences_controller.rb
index bac9b329d4..be4dc904d4 100644
--- a/app/controllers/settings/preferences_controller.rb
+++ b/app/controllers/settings/preferences_controller.rb
@@ -1,10 +1,6 @@
# frozen_string_literal: true
class Settings::PreferencesController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
-
def show; end
def update
diff --git a/app/controllers/settings/profiles_controller.rb b/app/controllers/settings/profiles_controller.rb
index 19a7ce157f..0c15447a6c 100644
--- a/app/controllers/settings/profiles_controller.rb
+++ b/app/controllers/settings/profiles_controller.rb
@@ -1,9 +1,6 @@
# frozen_string_literal: true
class Settings::ProfilesController < Settings::BaseController
- layout 'admin'
-
- before_action :authenticate_user!
before_action :set_account
def show
diff --git a/app/controllers/settings/sessions_controller.rb b/app/controllers/settings/sessions_controller.rb
index df5ace8036..ee2fc5dc80 100644
--- a/app/controllers/settings/sessions_controller.rb
+++ b/app/controllers/settings/sessions_controller.rb
@@ -1,11 +1,11 @@
# frozen_string_literal: true
class Settings::SessionsController < Settings::BaseController
- before_action :authenticate_user!
- before_action :set_session, only: :destroy
-
skip_before_action :require_functional!
+ before_action :require_not_suspended!
+ before_action :set_session, only: :destroy
+
def destroy
@session.destroy!
flash[:notice] = I18n.t('sessions.revoke_success')
diff --git a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb
index 9f23011a7d..1a0afe58b0 100644
--- a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb
+++ b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb
@@ -5,14 +5,11 @@ module Settings
class ConfirmationsController < BaseController
include ChallengableConcern
- layout 'admin'
+ skip_before_action :require_functional!
- before_action :authenticate_user!
before_action :require_challenge!
before_action :ensure_otp_secret
- skip_before_action :require_functional!
-
def new
prepare_two_factor_form
end
diff --git a/app/controllers/settings/two_factor_authentication/otp_authentication_controller.rb b/app/controllers/settings/two_factor_authentication/otp_authentication_controller.rb
index 6836f7ef62..cbba842a98 100644
--- a/app/controllers/settings/two_factor_authentication/otp_authentication_controller.rb
+++ b/app/controllers/settings/two_factor_authentication/otp_authentication_controller.rb
@@ -5,14 +5,11 @@ module Settings
class OtpAuthenticationController < BaseController
include ChallengableConcern
- layout 'admin'
+ skip_before_action :require_functional!
- before_action :authenticate_user!
before_action :verify_otp_not_enabled, only: [:show]
before_action :require_challenge!, only: [:create]
- skip_before_action :require_functional!
-
def show
@confirmation = Form::TwoFactorConfirmation.new
end
diff --git a/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb b/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb
index 0c4f5bff76..6ec53224d3 100644
--- a/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb
+++ b/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb
@@ -5,13 +5,10 @@ module Settings
class RecoveryCodesController < BaseController
include ChallengableConcern
- layout 'admin'
-
- before_action :authenticate_user!
- before_action :require_challenge!, on: :create
-
skip_before_action :require_functional!
+ before_action :require_challenge!, on: :create
+
def create
@recovery_codes = current_user.generate_otp_backup_codes!
current_user.save!
diff --git a/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb b/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb
index a19c604f3b..1c557092ba 100644
--- a/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb
+++ b/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb
@@ -3,9 +3,8 @@
module Settings
module TwoFactorAuthentication
class WebauthnCredentialsController < BaseController
- layout 'admin'
+ skip_before_action :require_functional!
- before_action :authenticate_user!
before_action :require_otp_enabled
before_action :require_webauthn_enabled, only: [:index, :destroy]
diff --git a/app/controllers/settings/two_factor_authentication_methods_controller.rb b/app/controllers/settings/two_factor_authentication_methods_controller.rb
index 224d3a45ca..205933ea81 100644
--- a/app/controllers/settings/two_factor_authentication_methods_controller.rb
+++ b/app/controllers/settings/two_factor_authentication_methods_controller.rb
@@ -4,14 +4,11 @@ module Settings
class TwoFactorAuthenticationMethodsController < BaseController
include ChallengableConcern
- layout 'admin'
+ skip_before_action :require_functional!
- before_action :authenticate_user!
before_action :require_challenge!, only: :disable
before_action :require_otp_enabled
- skip_before_action :require_functional!
-
def index; end
def disable
diff --git a/app/views/auth/registrations/_sessions.html.haml b/app/views/auth/registrations/_sessions.html.haml
index 395e36a9fd..d3a04c00e7 100644
--- a/app/views/auth/registrations/_sessions.html.haml
+++ b/app/views/auth/registrations/_sessions.html.haml
@@ -27,5 +27,5 @@
- else
%time.time-ago{ datetime: session.updated_at.iso8601, title: l(session.updated_at) }= l(session.updated_at)
%td
- - if current_session.session_id != session.session_id
+ - if current_session.session_id != session.session_id && !current_account.suspended?
= table_link_to 'times', t('sessions.revoke'), settings_session_path(session), method: :delete
diff --git a/app/views/auth/registrations/edit.html.haml b/app/views/auth/registrations/edit.html.haml
index 4a46b27a93..a3445b421a 100644
--- a/app/views/auth/registrations/edit.html.haml
+++ b/app/views/auth/registrations/edit.html.haml
@@ -30,18 +30,19 @@
= render 'sessions'
-%hr.spacer/
-
-%h3= t('auth.migrate_account')
-%p.muted-hint= t('auth.migrate_account_html', path: settings_migration_path)
-
-%hr.spacer/
-
-%h3= t('migrations.incoming_migrations')
-%p.muted-hint= t('migrations.incoming_migrations_html', path: settings_aliases_path)
-
-- if open_deletion? && !current_account.suspended?
+- unless current_account.suspended?
%hr.spacer/
- %h3= t('auth.delete_account')
- %p.muted-hint= t('auth.delete_account_html', path: settings_delete_path)
+ %h3= t('auth.migrate_account')
+ %p.muted-hint= t('auth.migrate_account_html', path: settings_migration_path)
+
+ %hr.spacer/
+
+ %h3= t('migrations.incoming_migrations')
+ %p.muted-hint= t('migrations.incoming_migrations_html', path: settings_aliases_path)
+
+ - if open_deletion?
+ %hr.spacer/
+
+ %h3= t('auth.delete_account')
+ %p.muted-hint= t('auth.delete_account_html', path: settings_delete_path)
diff --git a/app/views/oauth/authorized_applications/index.html.haml b/app/views/oauth/authorized_applications/index.html.haml
index 7b77108a93..fbb733db49 100644
--- a/app/views/oauth/authorized_applications/index.html.haml
+++ b/app/views/oauth/authorized_applications/index.html.haml
@@ -20,5 +20,5 @@
%th!= application.scopes.map { |scope| t(scope, scope: [:doorkeeper, :scopes]) }.join(', ')
%td= l application.created_at
%td
- - unless application.superapp?
+ - unless application.superapp? || current_account.suspended?
= table_link_to 'times', t('doorkeeper.authorized_applications.buttons.revoke'), oauth_authorized_application_path(application), method: :delete, data: { confirm: t('doorkeeper.authorized_applications.confirmations.revoke') }
diff --git a/config/navigation.rb b/config/navigation.rb
index ece41d4bf9..c113a3c3ee 100644
--- a/config/navigation.rb
+++ b/config/navigation.rb
@@ -21,7 +21,7 @@ SimpleNavigation::Configuration.run do |navigation|
n.item :security, safe_join([fa_icon('lock fw'), t('settings.account')]), edit_user_registration_url do |s|
s.item :password, safe_join([fa_icon('lock fw'), t('settings.account_settings')]), edit_user_registration_url, highlights_on: %r{/auth/edit|/settings/delete|/settings/migration|/settings/aliases}
- s.item :two_factor_authentication, safe_join([fa_icon('mobile fw'), t('settings.two_factor_authentication')]), settings_two_factor_authentication_methods_url, highlights_on: %r{/settings/two_factor_authentication|/settings/security_keys}
+ s.item :two_factor_authentication, safe_join([fa_icon('mobile fw'), t('settings.two_factor_authentication')]), settings_two_factor_authentication_methods_url, highlights_on: %r{/settings/two_factor_authentication|/settings/otp_authentication|/settings/security_keys}
s.item :authorized_apps, safe_join([fa_icon('list fw'), t('settings.authorized_apps')]), oauth_authorized_applications_url
end
diff --git a/spec/controllers/settings/deletes_controller_spec.rb b/spec/controllers/settings/deletes_controller_spec.rb
index 996872efd1..8d5c4774fd 100644
--- a/spec/controllers/settings/deletes_controller_spec.rb
+++ b/spec/controllers/settings/deletes_controller_spec.rb
@@ -77,6 +77,20 @@ describe Settings::DeletesController do
expect(response).to redirect_to settings_delete_path
end
end
+
+ context 'when account deletions are disabled' do
+ around do |example|
+ open_deletion = Setting.open_deletion
+ example.run
+ Setting.open_deletion = open_deletion
+ end
+
+ it 'redirects' do
+ Setting.open_deletion = false
+ delete :destroy
+ expect(response).to redirect_to root_path
+ end
+ end
end
context 'when not signed in' do
@@ -85,19 +99,5 @@ describe Settings::DeletesController do
expect(response).to redirect_to '/auth/sign_in'
end
end
-
- context do
- around do |example|
- open_deletion = Setting.open_deletion
- example.run
- Setting.open_deletion = open_deletion
- end
-
- it 'redirects' do
- Setting.open_deletion = false
- delete :destroy
- expect(response).to redirect_to root_path
- end
- end
end
end