Refactor settings controllers (#14767)
- Disallow suspended accounts from revoking sessions and apps - Allow suspended accounts to access exports
This commit is contained in:
		| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|  | ||||
|   | ||||
| @@ -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] | ||||
|  | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -2,7 +2,7 @@ | ||||
|  | ||||
| module Settings | ||||
|   module Exports | ||||
|     class BlockedAccountsController < ApplicationController | ||||
|     class BlockedAccountsController < BaseController | ||||
|       include ExportControllerConcern | ||||
|  | ||||
|       def index | ||||
|   | ||||
| @@ -2,7 +2,7 @@ | ||||
|  | ||||
| module Settings | ||||
|   module Exports | ||||
|     class BlockedDomainsController < ApplicationController | ||||
|     class BlockedDomainsController < BaseController | ||||
|       include ExportControllerConcern | ||||
|  | ||||
|       def index | ||||
|   | ||||
| @@ -2,7 +2,7 @@ | ||||
|  | ||||
| module Settings | ||||
|   module Exports | ||||
|     class FollowingAccountsController < ApplicationController | ||||
|     class FollowingAccountsController < BaseController | ||||
|       include ExportControllerConcern | ||||
|  | ||||
|       def index | ||||
|   | ||||
| @@ -2,7 +2,7 @@ | ||||
|  | ||||
| module Settings | ||||
|   module Exports | ||||
|     class ListsController < ApplicationController | ||||
|     class ListsController < BaseController | ||||
|       include ExportControllerConcern | ||||
|  | ||||
|       def index | ||||
|   | ||||
| @@ -2,7 +2,7 @@ | ||||
|  | ||||
| module Settings | ||||
|   module Exports | ||||
|     class MutedAccountsController < ApplicationController | ||||
|     class MutedAccountsController < BaseController | ||||
|       include ExportControllerConcern | ||||
|  | ||||
|       def index | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -2,7 +2,6 @@ | ||||
|  | ||||
| module Settings | ||||
|   class PicturesController < BaseController | ||||
|     before_action :authenticate_user! | ||||
|     before_action :set_account | ||||
|     before_action :set_picture | ||||
|  | ||||
|   | ||||
| @@ -1,10 +1,6 @@ | ||||
| # frozen_string_literal: true | ||||
|  | ||||
| class Settings::PreferencesController < Settings::BaseController | ||||
|   layout 'admin' | ||||
|  | ||||
|   before_action :authenticate_user! | ||||
|  | ||||
|   def show; end | ||||
|  | ||||
|   def update | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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') | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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! | ||||
|   | ||||
| @@ -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] | ||||
|  | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
| @@ -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) | ||||
|   | ||||
| @@ -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') } | ||||
|   | ||||
| @@ -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 | ||||
|  | ||||
|   | ||||
| @@ -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 | ||||
|   | ||||
		Reference in New Issue
	
	Block a user