From 44b2ee3485ba0845e5910cefcb4b1e2f84f34470 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 5 Jul 2022 02:41:40 +0200 Subject: Add customizable user roles (#18641) * Add customizable user roles * Various fixes and improvements * Add migration for old settings and fix tootctl role management --- .../account_moderation_notes_controller_spec.rb | 2 +- spec/controllers/admin/accounts_controller_spec.rb | 52 +++-- .../admin/action_logs_controller_spec.rb | 2 +- spec/controllers/admin/base_controller_spec.rb | 7 +- .../admin/change_email_controller_spec.rb | 2 +- .../admin/confirmations_controller_spec.rb | 2 +- .../admin/custom_emojis_controller_spec.rb | 2 +- .../controllers/admin/dashboard_controller_spec.rb | 2 +- .../admin/disputes/appeals_controller_spec.rb | 4 +- .../admin/domain_blocks_controller_spec.rb | 2 +- .../admin/email_domain_blocks_controller_spec.rb | 2 +- .../controllers/admin/instances_controller_spec.rb | 8 +- spec/controllers/admin/invites_controller_spec.rb | 2 +- .../admin/report_notes_controller_spec.rb | 2 +- spec/controllers/admin/reports_controller_spec.rb | 2 +- spec/controllers/admin/resets_controller_spec.rb | 2 +- spec/controllers/admin/roles_controller_spec.rb | 244 +++++++++++++++++++-- spec/controllers/admin/settings_controller_spec.rb | 2 +- spec/controllers/admin/statuses_controller_spec.rb | 2 +- spec/controllers/admin/tags_controller_spec.rb | 2 +- .../two_factor_authentications_controller_spec.rb | 51 ----- spec/controllers/admin/users/roles_controller.rb | 81 +++++++ .../two_factor_authentications_controller_spec.rb | 52 +++++ .../v1/admin/account_actions_controller_spec.rb | 6 +- .../api/v1/admin/accounts_controller_spec.rb | 20 +- .../api/v1/admin/domain_allows_controller_spec.rb | 20 +- .../api/v1/admin/domain_blocks_controller_spec.rb | 20 +- .../api/v1/admin/reports_controller_spec.rb | 16 +- spec/controllers/api/v1/reports_controller_spec.rb | 2 +- .../api/v2/admin/accounts_controller_spec.rb | 6 +- spec/controllers/application_controller_spec.rb | 64 ------ .../disputes/appeals_controller_spec.rb | 2 +- spec/controllers/invites_controller_spec.rb | 40 ++-- spec/fabricators/user_role_fabricator.rb | 5 + spec/models/account_spec.rb | 14 +- spec/models/admin/account_action_spec.rb | 2 +- spec/models/user_role_spec.rb | 189 ++++++++++++++++ spec/models/user_spec.rb | 159 +------------- .../account_moderation_note_policy_spec.rb | 4 +- spec/policies/account_policy_spec.rb | 8 +- spec/policies/custom_emoji_policy_spec.rb | 2 +- spec/policies/domain_block_policy_spec.rb | 2 +- spec/policies/email_domain_block_policy_spec.rb | 2 +- spec/policies/instance_policy_spec.rb | 2 +- spec/policies/invite_policy_spec.rb | 54 ++--- spec/policies/relay_policy_spec.rb | 2 +- spec/policies/report_note_policy_spec.rb | 5 +- spec/policies/report_policy_spec.rb | 2 +- spec/policies/settings_policy_spec.rb | 2 +- spec/policies/status_policy_spec.rb | 2 +- spec/policies/tag_policy_spec.rb | 2 +- spec/policies/user_policy_spec.rb | 55 +---- 52 files changed, 729 insertions(+), 509 deletions(-) delete mode 100644 spec/controllers/admin/two_factor_authentications_controller_spec.rb create mode 100644 spec/controllers/admin/users/roles_controller.rb create mode 100644 spec/controllers/admin/users/two_factor_authentications_controller_spec.rb create mode 100644 spec/fabricators/user_role_fabricator.rb create mode 100644 spec/models/user_role_spec.rb (limited to 'spec') diff --git a/spec/controllers/admin/account_moderation_notes_controller_spec.rb b/spec/controllers/admin/account_moderation_notes_controller_spec.rb index 410ce6543..d3f3263f8 100644 --- a/spec/controllers/admin/account_moderation_notes_controller_spec.rb +++ b/spec/controllers/admin/account_moderation_notes_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Admin::AccountModerationNotesController, type: :controller do render_views - let(:user) { Fabricate(:user, admin: true) } + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } let(:target_account) { Fabricate(:account) } before do diff --git a/spec/controllers/admin/accounts_controller_spec.rb b/spec/controllers/admin/accounts_controller_spec.rb index 1779fb7c0..1bd51a0c8 100644 --- a/spec/controllers/admin/accounts_controller_spec.rb +++ b/spec/controllers/admin/accounts_controller_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Admin::AccountsController, type: :controller do before { sign_in current_user, scope: :user } describe 'GET #index' do - let(:current_user) { Fabricate(:user, admin: true) } + let(:current_user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } around do |example| default_per_page = Account.default_per_page @@ -60,7 +60,7 @@ RSpec.describe Admin::AccountsController, type: :controller do end describe 'GET #show' do - let(:current_user) { Fabricate(:user, admin: true) } + let(:current_user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } let(:account) { Fabricate(:account) } it 'returns http success' do @@ -72,15 +72,15 @@ RSpec.describe Admin::AccountsController, type: :controller do describe 'POST #memorialize' do subject { post :memorialize, params: { id: account.id } } - let(:current_user) { Fabricate(:user, admin: current_user_admin) } + let(:current_user) { Fabricate(:user, role: current_role) } let(:account) { user.account } - let(:user) { Fabricate(:user, admin: target_user_admin) } + let(:user) { Fabricate(:user, role: target_role) } context 'when user is admin' do - let(:current_user_admin) { true } + let(:current_role) { UserRole.find_by(name: 'Admin') } context 'when target user is admin' do - let(:target_user_admin) { true } + let(:target_role) { UserRole.find_by(name: 'Admin') } it 'fails to memorialize account' do is_expected.to have_http_status :forbidden @@ -89,7 +89,7 @@ RSpec.describe Admin::AccountsController, type: :controller do end context 'when target user is not admin' do - let(:target_user_admin) { false } + let(:target_role) { UserRole.find_by(name: 'Moderator') } it 'succeeds in memorializing account' do is_expected.to redirect_to admin_account_path(account.id) @@ -99,10 +99,10 @@ RSpec.describe Admin::AccountsController, type: :controller do end context 'when user is not admin' do - let(:current_user_admin) { false } + let(:current_role) { UserRole.find_by(name: 'Moderator') } context 'when target user is admin' do - let(:target_user_admin) { true } + let(:target_role) { UserRole.find_by(name: 'Admin') } it 'fails to memorialize account' do is_expected.to have_http_status :forbidden @@ -111,7 +111,7 @@ RSpec.describe Admin::AccountsController, type: :controller do end context 'when target user is not admin' do - let(:target_user_admin) { false } + let(:target_role) { UserRole.find_by(name: 'Moderator') } it 'fails to memorialize account' do is_expected.to have_http_status :forbidden @@ -124,12 +124,12 @@ RSpec.describe Admin::AccountsController, type: :controller do describe 'POST #enable' do subject { post :enable, params: { id: account.id } } - let(:current_user) { Fabricate(:user, admin: admin) } + let(:current_user) { Fabricate(:user, role: role) } let(:account) { user.account } let(:user) { Fabricate(:user, disabled: true) } context 'when user is admin' do - let(:admin) { true } + let(:role) { UserRole.find_by(name: 'Admin') } it 'succeeds in enabling account' do is_expected.to redirect_to admin_account_path(account.id) @@ -138,7 +138,7 @@ RSpec.describe Admin::AccountsController, type: :controller do end context 'when user is not admin' do - let(:admin) { false } + let(:role) { UserRole.everyone } it 'fails to enable account' do is_expected.to have_http_status :forbidden @@ -150,19 +150,23 @@ RSpec.describe Admin::AccountsController, type: :controller do describe 'POST #redownload' do subject { post :redownload, params: { id: account.id } } - let(:current_user) { Fabricate(:user, admin: admin) } - let(:account) { Fabricate(:account) } + let(:current_user) { Fabricate(:user, role: role) } + let(:account) { Fabricate(:account, domain: 'example.com') } + + before do + allow_any_instance_of(ResolveAccountService).to receive(:call) + end context 'when user is admin' do - let(:admin) { true } + let(:role) { UserRole.find_by(name: 'Admin') } - it 'succeeds in redownloadin' do + it 'succeeds in redownloading' do is_expected.to redirect_to admin_account_path(account.id) end end context 'when user is not admin' do - let(:admin) { false } + let(:role) { UserRole.everyone } it 'fails to redownload' do is_expected.to have_http_status :forbidden @@ -173,11 +177,11 @@ RSpec.describe Admin::AccountsController, type: :controller do describe 'POST #remove_avatar' do subject { post :remove_avatar, params: { id: account.id } } - let(:current_user) { Fabricate(:user, admin: admin) } + let(:current_user) { Fabricate(:user, role: role) } let(:account) { Fabricate(:account) } context 'when user is admin' do - let(:admin) { true } + let(:role) { UserRole.find_by(name: 'Admin') } it 'succeeds in removing avatar' do is_expected.to redirect_to admin_account_path(account.id) @@ -185,7 +189,7 @@ RSpec.describe Admin::AccountsController, type: :controller do end context 'when user is not admin' do - let(:admin) { false } + let(:role) { UserRole.everyone } it 'fails to remove avatar' do is_expected.to have_http_status :forbidden @@ -196,12 +200,12 @@ RSpec.describe Admin::AccountsController, type: :controller do describe 'POST #unblock_email' do subject { post :unblock_email, params: { id: account.id } } - let(:current_user) { Fabricate(:user, admin: admin) } + let(:current_user) { Fabricate(:user, role: role) } let(:account) { Fabricate(:account, suspended: true) } let!(:email_block) { Fabricate(:canonical_email_block, reference_account: account) } context 'when user is admin' do - let(:admin) { true } + let(:role) { UserRole.find_by(name: 'Admin') } it 'succeeds in removing email blocks' do expect { subject }.to change { CanonicalEmailBlock.where(reference_account: account).count }.from(1).to(0) @@ -214,7 +218,7 @@ RSpec.describe Admin::AccountsController, type: :controller do end context 'when user is not admin' do - let(:admin) { false } + let(:role) { UserRole.everyone } it 'fails to remove avatar' do subject diff --git a/spec/controllers/admin/action_logs_controller_spec.rb b/spec/controllers/admin/action_logs_controller_spec.rb index 4720ed2e2..c1957258f 100644 --- a/spec/controllers/admin/action_logs_controller_spec.rb +++ b/spec/controllers/admin/action_logs_controller_spec.rb @@ -5,7 +5,7 @@ require 'rails_helper' describe Admin::ActionLogsController, type: :controller do describe 'GET #index' do it 'returns 200' do - sign_in Fabricate(:user, admin: true) + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')) get :index, params: { page: 1 } expect(response).to have_http_status(200) diff --git a/spec/controllers/admin/base_controller_spec.rb b/spec/controllers/admin/base_controller_spec.rb index 9ac833623..44be91951 100644 --- a/spec/controllers/admin/base_controller_spec.rb +++ b/spec/controllers/admin/base_controller_spec.rb @@ -5,13 +5,14 @@ require 'rails_helper' describe Admin::BaseController, type: :controller do controller do def success + authorize :dashboard, :index? render 'admin/reports/show' end end it 'requires administrator or moderator' do routes.draw { get 'success' => 'admin/base#success' } - sign_in(Fabricate(:user, admin: false, moderator: false)) + sign_in(Fabricate(:user)) get :success expect(response).to have_http_status(:forbidden) @@ -19,14 +20,14 @@ describe Admin::BaseController, type: :controller do it 'renders admin layout as a moderator' do routes.draw { get 'success' => 'admin/base#success' } - sign_in(Fabricate(:user, moderator: true)) + sign_in(Fabricate(:user, role: UserRole.find_by(name: 'Moderator'))) get :success expect(response).to render_template layout: 'admin' end it 'renders admin layout as an admin' do routes.draw { get 'success' => 'admin/base#success' } - sign_in(Fabricate(:user, admin: true)) + sign_in(Fabricate(:user, role: UserRole.find_by(name: 'Admin'))) get :success expect(response).to render_template layout: 'admin' end diff --git a/spec/controllers/admin/change_email_controller_spec.rb b/spec/controllers/admin/change_email_controller_spec.rb index e7f3f7c97..cf8a27d39 100644 --- a/spec/controllers/admin/change_email_controller_spec.rb +++ b/spec/controllers/admin/change_email_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Admin::ChangeEmailsController, type: :controller do render_views - let(:admin) { Fabricate(:user, admin: true) } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do sign_in admin diff --git a/spec/controllers/admin/confirmations_controller_spec.rb b/spec/controllers/admin/confirmations_controller_spec.rb index 5b4f7e925..6268903c4 100644 --- a/spec/controllers/admin/confirmations_controller_spec.rb +++ b/spec/controllers/admin/confirmations_controller_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Admin::ConfirmationsController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'POST #create' do diff --git a/spec/controllers/admin/custom_emojis_controller_spec.rb b/spec/controllers/admin/custom_emojis_controller_spec.rb index a8d96948c..06cd0c22d 100644 --- a/spec/controllers/admin/custom_emojis_controller_spec.rb +++ b/spec/controllers/admin/custom_emojis_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe Admin::CustomEmojisController do render_views - let(:user) { Fabricate(:user, admin: true) } + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do sign_in user, scope: :user diff --git a/spec/controllers/admin/dashboard_controller_spec.rb b/spec/controllers/admin/dashboard_controller_spec.rb index 7824854f9..6231a09a2 100644 --- a/spec/controllers/admin/dashboard_controller_spec.rb +++ b/spec/controllers/admin/dashboard_controller_spec.rb @@ -12,7 +12,7 @@ describe Admin::DashboardController, type: :controller do Admin::SystemCheck::Message.new(:rules_check, nil, admin_rules_path), Admin::SystemCheck::Message.new(:sidekiq_process_check, 'foo, bar'), ]) - sign_in Fabricate(:user, admin: true) + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')) end it 'returns 200' do diff --git a/spec/controllers/admin/disputes/appeals_controller_spec.rb b/spec/controllers/admin/disputes/appeals_controller_spec.rb index 6a06f9406..712657791 100644 --- a/spec/controllers/admin/disputes/appeals_controller_spec.rb +++ b/spec/controllers/admin/disputes/appeals_controller_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Admin::Disputes::AppealsController, type: :controller do end describe 'POST #approve' do - let(:current_user) { Fabricate(:user, admin: true) } + let(:current_user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do allow(UserMailer).to receive(:appeal_approved).and_return(double('email', deliver_later: nil)) @@ -35,7 +35,7 @@ RSpec.describe Admin::Disputes::AppealsController, type: :controller do end describe 'POST #reject' do - let(:current_user) { Fabricate(:user, admin: true) } + let(:current_user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do allow(UserMailer).to receive(:appeal_rejected).and_return(double('email', deliver_later: nil)) diff --git a/spec/controllers/admin/domain_blocks_controller_spec.rb b/spec/controllers/admin/domain_blocks_controller_spec.rb index ecc79292b..5c2dcd268 100644 --- a/spec/controllers/admin/domain_blocks_controller_spec.rb +++ b/spec/controllers/admin/domain_blocks_controller_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Admin::DomainBlocksController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'GET #new' do diff --git a/spec/controllers/admin/email_domain_blocks_controller_spec.rb b/spec/controllers/admin/email_domain_blocks_controller_spec.rb index cf194579d..e9cef4a94 100644 --- a/spec/controllers/admin/email_domain_blocks_controller_spec.rb +++ b/spec/controllers/admin/email_domain_blocks_controller_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Admin::EmailDomainBlocksController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'GET #index' do diff --git a/spec/controllers/admin/instances_controller_spec.rb b/spec/controllers/admin/instances_controller_spec.rb index 53427b874..337f7a80c 100644 --- a/spec/controllers/admin/instances_controller_spec.rb +++ b/spec/controllers/admin/instances_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Admin::InstancesController, type: :controller do render_views - let(:current_user) { Fabricate(:user, admin: true) } + let(:current_user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } let!(:account) { Fabricate(:account, domain: 'popular') } let!(:account2) { Fabricate(:account, domain: 'popular') } @@ -35,11 +35,11 @@ RSpec.describe Admin::InstancesController, type: :controller do describe 'DELETE #destroy' do subject { delete :destroy, params: { id: Instance.first.id } } - let(:current_user) { Fabricate(:user, admin: admin) } + let(:current_user) { Fabricate(:user, role: role) } let(:account) { Fabricate(:account) } context 'when user is admin' do - let(:admin) { true } + let(:role) { UserRole.find_by(name: 'Admin') } it 'succeeds in purging instance' do is_expected.to redirect_to admin_instances_path @@ -47,7 +47,7 @@ RSpec.describe Admin::InstancesController, type: :controller do end context 'when user is not admin' do - let(:admin) { false } + let(:role) { nil } it 'fails to purge instance' do is_expected.to have_http_status :forbidden diff --git a/spec/controllers/admin/invites_controller_spec.rb b/spec/controllers/admin/invites_controller_spec.rb index 449a699e4..1fb488742 100644 --- a/spec/controllers/admin/invites_controller_spec.rb +++ b/spec/controllers/admin/invites_controller_spec.rb @@ -5,7 +5,7 @@ require 'rails_helper' describe Admin::InvitesController do render_views - let(:user) { Fabricate(:user, admin: true) } + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do sign_in user, scope: :user diff --git a/spec/controllers/admin/report_notes_controller_spec.rb b/spec/controllers/admin/report_notes_controller_spec.rb index c0013f41a..fa7572d18 100644 --- a/spec/controllers/admin/report_notes_controller_spec.rb +++ b/spec/controllers/admin/report_notes_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe Admin::ReportNotesController do render_views - let(:user) { Fabricate(:user, admin: true) } + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do sign_in user, scope: :user diff --git a/spec/controllers/admin/reports_controller_spec.rb b/spec/controllers/admin/reports_controller_spec.rb index d421f0739..4cd1524bf 100644 --- a/spec/controllers/admin/reports_controller_spec.rb +++ b/spec/controllers/admin/reports_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe Admin::ReportsController do render_views - let(:user) { Fabricate(:user, admin: true) } + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } before do sign_in user, scope: :user end diff --git a/spec/controllers/admin/resets_controller_spec.rb b/spec/controllers/admin/resets_controller_spec.rb index 28510b5af..aeb172318 100644 --- a/spec/controllers/admin/resets_controller_spec.rb +++ b/spec/controllers/admin/resets_controller_spec.rb @@ -5,7 +5,7 @@ describe Admin::ResetsController do let(:account) { Fabricate(:account) } before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'POST #create' do diff --git a/spec/controllers/admin/roles_controller_spec.rb b/spec/controllers/admin/roles_controller_spec.rb index 8e0de73cb..8ff891205 100644 --- a/spec/controllers/admin/roles_controller_spec.rb +++ b/spec/controllers/admin/roles_controller_spec.rb @@ -3,31 +3,247 @@ require 'rails_helper' describe Admin::RolesController do render_views - let(:admin) { Fabricate(:user, admin: true) } + let(:permissions) { UserRole::Flags::NONE } + let(:current_role) { UserRole.create(name: 'Foo', permissions: permissions, position: 10) } + let(:current_user) { Fabricate(:user, role: current_role) } before do - sign_in admin, scope: :user + sign_in current_user, scope: :user end - describe 'POST #promote' do - subject { post :promote, params: { account_id: user.account_id } } + describe 'GET #index' do + before do + get :index + end + + context 'when user does not have permission to manage roles' do + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end - let(:user) { Fabricate(:user, moderator: false, admin: false) } + context 'when user has permission to manage roles' do + let(:permissions) { UserRole::FLAGS[:manage_roles] } - it 'promotes user' do - expect(subject).to redirect_to admin_account_path(user.account_id) - expect(user.reload).to be_moderator + it 'returns http success' do + expect(response).to have_http_status(:success) + end end end - describe 'POST #demote' do - subject { post :demote, params: { account_id: user.account_id } } + describe 'GET #new' do + before do + get :new + end + + context 'when user does not have permission to manage roles' do + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end + + context 'when user has permission to manage roles' do + let(:permissions) { UserRole::FLAGS[:manage_roles] } + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + end + end + + describe 'POST #create' do + let(:selected_position) { 1 } + let(:selected_permissions_as_keys) { %w(manage_roles) } + + before do + post :create, params: { user_role: { name: 'Bar', position: selected_position, permissions_as_keys: selected_permissions_as_keys } } + end + + context 'when user has permission to manage roles' do + let(:permissions) { UserRole::FLAGS[:manage_roles] } + + context 'when new role\'s does not elevate above the user\'s role' do + let(:selected_position) { 1 } + let(:selected_permissions_as_keys) { %w(manage_roles) } + + it 'redirects to roles page' do + expect(response).to redirect_to(admin_roles_path) + end + + it 'creates new role' do + expect(UserRole.find_by(name: 'Bar')).to_not be_nil + end + end + + context 'when new role\'s position is higher than user\'s role' do + let(:selected_position) { 100 } + let(:selected_permissions_as_keys) { %w(manage_roles) } + + it 'renders new template' do + expect(response).to render_template(:new) + end + + it 'does not create new role' do + expect(UserRole.find_by(name: 'Bar')).to be_nil + end + end + + context 'when new role has permissions the user does not have' do + let(:selected_position) { 1 } + let(:selected_permissions_as_keys) { %w(manage_roles manage_users manage_reports) } + + it 'renders new template' do + expect(response).to render_template(:new) + end + + it 'does not create new role' do + expect(UserRole.find_by(name: 'Bar')).to be_nil + end + end + + context 'when user has administrator permission' do + let(:permissions) { UserRole::FLAGS[:administrator] } + + let(:selected_position) { 1 } + let(:selected_permissions_as_keys) { %w(manage_roles manage_users manage_reports) } + + it 'redirects to roles page' do + expect(response).to redirect_to(admin_roles_path) + end + + it 'creates new role' do + expect(UserRole.find_by(name: 'Bar')).to_not be_nil + end + end + end + end + + describe 'GET #edit' do + let(:role_position) { 8 } + let(:role) { UserRole.create(name: 'Bar', permissions: UserRole::FLAGS[:manage_users], position: role_position) } + + before do + get :edit, params: { id: role.id } + end + + context 'when user does not have permission to manage roles' do + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end + + context 'when user has permission to manage roles' do + let(:permissions) { UserRole::FLAGS[:manage_roles] } + + context 'when user outranks the role' do + it 'returns http success' do + expect(response).to have_http_status(:success) + end + end + + context 'when role outranks user' do + let(:role_position) { current_role.position + 1 } + + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end + end + end + + describe 'PUT #update' do + let(:role_position) { 8 } + let(:role_permissions) { UserRole::FLAGS[:manage_users] } + let(:role) { UserRole.create(name: 'Bar', permissions: role_permissions, position: role_position) } + + let(:selected_position) { 8 } + let(:selected_permissions_as_keys) { %w(manage_users) } + + before do + put :update, params: { id: role.id, user_role: { name: 'Baz', position: selected_position, permissions_as_keys: selected_permissions_as_keys } } + end + + context 'when user does not have permission to manage roles' do + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not update the role' do + expect(role.reload.name).to eq 'Bar' + end + end + + context 'when user has permission to manage roles' do + let(:permissions) { UserRole::FLAGS[:manage_roles] } + + context 'when role has permissions the user doesn\'t' do + it 'renders edit template' do + expect(response).to render_template(:edit) + end + + it 'does not update the role' do + expect(role.reload.name).to eq 'Bar' + end + end + + context 'when user has all permissions of the role' do + let(:permissions) { UserRole::FLAGS[:manage_roles] | UserRole::FLAGS[:manage_users] } + + context 'when user outranks the role' do + it 'redirects to roles page' do + expect(response).to redirect_to(admin_roles_path) + end + + it 'updates the role' do + expect(role.reload.name).to eq 'Baz' + end + end + + context 'when role outranks user' do + let(:role_position) { current_role.position + 1 } + + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not update the role' do + expect(role.reload.name).to eq 'Bar' + end + end + end + end + end + + describe 'DELETE #destroy' do + let(:role_position) { 8 } + let(:role) { UserRole.create(name: 'Bar', permissions: UserRole::FLAGS[:manage_users], position: role_position) } + + before do + delete :destroy, params: { id: role.id } + end + + context 'when user does not have permission to manage roles' do + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end + + context 'when user has permission to manage roles' do + let(:permissions) { UserRole::FLAGS[:manage_roles] } + + context 'when user outranks the role' do + it 'redirects to roles page' do + expect(response).to redirect_to(admin_roles_path) + end + end - let(:user) { Fabricate(:user, moderator: true, admin: false) } + context 'when role outranks user' do + let(:role_position) { current_role.position + 1 } - it 'demotes user' do - expect(subject).to redirect_to admin_account_path(user.account_id) - expect(user.reload).not_to be_moderator + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end end end end diff --git a/spec/controllers/admin/settings_controller_spec.rb b/spec/controllers/admin/settings_controller_spec.rb index 6cf0ee20a..46749f76c 100644 --- a/spec/controllers/admin/settings_controller_spec.rb +++ b/spec/controllers/admin/settings_controller_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Admin::SettingsController, type: :controller do describe 'When signed in as an admin' do before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'GET #edit' do diff --git a/spec/controllers/admin/statuses_controller_spec.rb b/spec/controllers/admin/statuses_controller_spec.rb index de32fd18e..227688e23 100644 --- a/spec/controllers/admin/statuses_controller_spec.rb +++ b/spec/controllers/admin/statuses_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe Admin::StatusesController do render_views - let(:user) { Fabricate(:user, admin: true) } + let(:user) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } let(:account) { Fabricate(:account) } let!(:status) { Fabricate(:status, account: account) } let(:media_attached_status) { Fabricate(:status, account: account, sensitive: !sensitive) } diff --git a/spec/controllers/admin/tags_controller_spec.rb b/spec/controllers/admin/tags_controller_spec.rb index 85c801a9c..52fd09eb1 100644 --- a/spec/controllers/admin/tags_controller_spec.rb +++ b/spec/controllers/admin/tags_controller_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Admin::TagsController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true) + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')) end describe 'GET #show' do diff --git a/spec/controllers/admin/two_factor_authentications_controller_spec.rb b/spec/controllers/admin/two_factor_authentications_controller_spec.rb deleted file mode 100644 index c65095729..000000000 --- a/spec/controllers/admin/two_factor_authentications_controller_spec.rb +++ /dev/null @@ -1,51 +0,0 @@ -require 'rails_helper' -require 'webauthn/fake_client' - -describe Admin::TwoFactorAuthenticationsController do - render_views - - let(:user) { Fabricate(:user) } - before do - sign_in Fabricate(:user, admin: true), scope: :user - end - - describe 'DELETE #destroy' do - context 'when user has OTP enabled' do - before do - user.update(otp_required_for_login: true) - end - - it 'redirects to admin account page' do - delete :destroy, params: { user_id: user.id } - - user.reload - expect(user.otp_enabled?).to eq false - expect(response).to redirect_to(admin_account_path(user.account_id)) - end - end - - context 'when user has OTP and WebAuthn enabled' do - let(:fake_client) { WebAuthn::FakeClient.new('http://test.host') } - - before do - user.update(otp_required_for_login: true, webauthn_id: WebAuthn.generate_user_id) - - public_key_credential = WebAuthn::Credential.from_create(fake_client.create) - Fabricate(:webauthn_credential, - user_id: user.id, - external_id: public_key_credential.id, - public_key: public_key_credential.public_key, - nickname: 'Security Key') - end - - it 'redirects to admin account page' do - delete :destroy, params: { user_id: user.id } - - user.reload - expect(user.otp_enabled?).to eq false - expect(user.webauthn_enabled?).to eq false - expect(response).to redirect_to(admin_account_path(user.account_id)) - end - end - end -end diff --git a/spec/controllers/admin/users/roles_controller.rb b/spec/controllers/admin/users/roles_controller.rb new file mode 100644 index 000000000..bd6a3fa67 --- /dev/null +++ b/spec/controllers/admin/users/roles_controller.rb @@ -0,0 +1,81 @@ +require 'rails_helper' + +describe Admin::Users::RolesController do + render_views + + let(:current_role) { UserRole.create(name: 'Foo', permissions: UserRole::FLAGS[:manage_roles], position: 10) } + let(:current_user) { Fabricate(:user, role: current_role) } + + let(:previous_role) { nil } + let(:user) { Fabricate(:user, role: previous_role) } + + before do + sign_in current_user, scope: :user + end + + describe 'GET #show' do + before do + get :show, params: { user_id: user.id } + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + context 'when target user is higher ranked than current user' do + let(:previous_role) { UserRole.create(name: 'Baz', permissions: UserRole::FLAGS[:administrator], position: 100) } + + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end + end + + describe 'PUT #update' do + let(:selected_role) { UserRole.create(name: 'Bar', permissions: permissions, position: position) } + + before do + put :update, params: { user_id: user.id, user: { role_id: selected_role.id } } + end + + context do + let(:permissions) { UserRole::FLAGS[:manage_roles] } + let(:position) { 1 } + + it 'updates user role' do + expect(user.reload.role_id).to eq selected_role&.id + end + + it 'redirects back to account page' do + expect(response).to redirect_to(admin_account_path(user.account_id)) + end + end + + context 'when selected role has higher position than current user\'s role' do + let(:permissions) { UserRole::FLAGS[:administrator] } + let(:position) { 100 } + + it 'does not update user role' do + expect(user.reload.role_id).to eq previous_role&.id + end + + it 'renders edit form' do + expect(response).to render_template(:show) + end + end + + context 'when target user is higher ranked than current user' do + let(:previous_role) { UserRole.create(name: 'Baz', permissions: UserRole::FLAGS[:administrator], position: 100) } + let(:permissions) { UserRole::FLAGS[:manage_roles] } + let(:position) { 1 } + + it 'does not update user role' do + expect(user.reload.role_id).to eq previous_role&.id + end + + it 'returns http forbidden' do + expect(response).to have_http_status(:forbidden) + end + end + end +end diff --git a/spec/controllers/admin/users/two_factor_authentications_controller_spec.rb b/spec/controllers/admin/users/two_factor_authentications_controller_spec.rb new file mode 100644 index 000000000..e56264ef6 --- /dev/null +++ b/spec/controllers/admin/users/two_factor_authentications_controller_spec.rb @@ -0,0 +1,52 @@ +require 'rails_helper' +require 'webauthn/fake_client' + +describe Admin::Users::TwoFactorAuthenticationsController do + render_views + + let(:user) { Fabricate(:user) } + + before do + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user + end + + describe 'DELETE #destroy' do + context 'when user has OTP enabled' do + before do + user.update(otp_required_for_login: true) + end + + it 'redirects to admin account page' do + delete :destroy, params: { user_id: user.id } + + user.reload + expect(user.otp_enabled?).to eq false + expect(response).to redirect_to(admin_account_path(user.account_id)) + end + end + + context 'when user has OTP and WebAuthn enabled' do + let(:fake_client) { WebAuthn::FakeClient.new('http://test.host') } + + before do + user.update(otp_required_for_login: true, webauthn_id: WebAuthn.generate_user_id) + + public_key_credential = WebAuthn::Credential.from_create(fake_client.create) + Fabricate(:webauthn_credential, + user_id: user.id, + external_id: public_key_credential.id, + public_key: public_key_credential.public_key, + nickname: 'Security Key') + end + + it 'redirects to admin account page' do + delete :destroy, params: { user_id: user.id } + + user.reload + expect(user.otp_enabled?).to eq false + expect(user.webauthn_enabled?).to eq false + expect(response).to redirect_to(admin_account_path(user.account_id)) + end + end + end +end diff --git a/spec/controllers/api/v1/admin/account_actions_controller_spec.rb b/spec/controllers/api/v1/admin/account_actions_controller_spec.rb index 601290b82..199395f55 100644 --- a/spec/controllers/api/v1/admin/account_actions_controller_spec.rb +++ b/spec/controllers/api/v1/admin/account_actions_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Api::V1::Admin::AccountActionsController, type: :controller do render_views - let(:role) { 'moderator' } + let(:role) { UserRole.find_by(name: 'Moderator') } let(:user) { Fabricate(:user, role: role) } let(:scopes) { 'admin:read admin:write' } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } @@ -22,7 +22,7 @@ RSpec.describe Api::V1::Admin::AccountActionsController, type: :controller do end shared_examples 'forbidden for wrong role' do |wrong_role| - let(:role) { wrong_role } + let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do expect(response).to have_http_status(403) @@ -35,7 +35,7 @@ RSpec.describe Api::V1::Admin::AccountActionsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) diff --git a/spec/controllers/api/v1/admin/accounts_controller_spec.rb b/spec/controllers/api/v1/admin/accounts_controller_spec.rb index b69595f7e..cd38030e0 100644 --- a/spec/controllers/api/v1/admin/accounts_controller_spec.rb +++ b/spec/controllers/api/v1/admin/accounts_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Api::V1::Admin::AccountsController, type: :controller do render_views - let(:role) { 'moderator' } + let(:role) { UserRole.find_by(name: 'Moderator') } let(:user) { Fabricate(:user, role: role) } let(:scopes) { 'admin:read admin:write' } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } @@ -22,7 +22,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end shared_examples 'forbidden for wrong role' do |wrong_role| - let(:role) { wrong_role } + let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do expect(response).to have_http_status(403) @@ -46,7 +46,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' [ [{ active: 'true', local: 'true', staff: 'true' }, [:admin_account]], @@ -77,7 +77,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -91,7 +91,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -109,7 +109,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -127,7 +127,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -145,7 +145,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -163,7 +163,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -181,7 +181,7 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) diff --git a/spec/controllers/api/v1/admin/domain_allows_controller_spec.rb b/spec/controllers/api/v1/admin/domain_allows_controller_spec.rb index edee3ab6c..26a391a60 100644 --- a/spec/controllers/api/v1/admin/domain_allows_controller_spec.rb +++ b/spec/controllers/api/v1/admin/domain_allows_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Api::V1::Admin::DomainAllowsController, type: :controller do render_views - let(:role) { 'admin' } + let(:role) { UserRole.find_by(name: 'Admin') } let(:user) { Fabricate(:user, role: role) } let(:scopes) { 'admin:read admin:write' } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } @@ -21,7 +21,7 @@ RSpec.describe Api::V1::Admin::DomainAllowsController, type: :controller do end shared_examples 'forbidden for wrong role' do |wrong_role| - let(:role) { wrong_role } + let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do expect(response).to have_http_status(403) @@ -36,8 +36,8 @@ RSpec.describe Api::V1::Admin::DomainAllowsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) @@ -58,8 +58,8 @@ RSpec.describe Api::V1::Admin::DomainAllowsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) @@ -79,8 +79,8 @@ RSpec.describe Api::V1::Admin::DomainAllowsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) @@ -99,8 +99,8 @@ RSpec.describe Api::V1::Admin::DomainAllowsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) diff --git a/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb b/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb index 196f6dc28..f12285b2a 100644 --- a/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb +++ b/spec/controllers/api/v1/admin/domain_blocks_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do render_views - let(:role) { 'admin' } + let(:role) { UserRole.find_by(name: 'Admin') } let(:user) { Fabricate(:user, role: role) } let(:scopes) { 'admin:read admin:write' } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } @@ -21,7 +21,7 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do end shared_examples 'forbidden for wrong role' do |wrong_role| - let(:role) { wrong_role } + let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do expect(response).to have_http_status(403) @@ -36,8 +36,8 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) @@ -58,8 +58,8 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) @@ -79,8 +79,8 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) @@ -100,8 +100,8 @@ RSpec.describe Api::V1::Admin::DomainBlocksController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' - it_behaves_like 'forbidden for wrong role', 'moderator' + it_behaves_like 'forbidden for wrong role', '' + it_behaves_like 'forbidden for wrong role', 'Moderator' it 'returns http success' do expect(response).to have_http_status(200) diff --git a/spec/controllers/api/v1/admin/reports_controller_spec.rb b/spec/controllers/api/v1/admin/reports_controller_spec.rb index b6df53048..880e72030 100644 --- a/spec/controllers/api/v1/admin/reports_controller_spec.rb +++ b/spec/controllers/api/v1/admin/reports_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Api::V1::Admin::ReportsController, type: :controller do render_views - let(:role) { 'moderator' } + let(:role) { UserRole.find_by(name: 'Moderator') } let(:user) { Fabricate(:user, role: role) } let(:scopes) { 'admin:read admin:write' } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } @@ -22,7 +22,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end shared_examples 'forbidden for wrong role' do |wrong_role| - let(:role) { wrong_role } + let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do expect(response).to have_http_status(403) @@ -35,7 +35,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -48,7 +48,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -61,7 +61,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -74,7 +74,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -87,7 +87,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) @@ -100,7 +100,7 @@ RSpec.describe Api::V1::Admin::ReportsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' it 'returns http success' do expect(response).to have_http_status(200) diff --git a/spec/controllers/api/v1/reports_controller_spec.rb b/spec/controllers/api/v1/reports_controller_spec.rb index b5baf60e1..dbc64e704 100644 --- a/spec/controllers/api/v1/reports_controller_spec.rb +++ b/spec/controllers/api/v1/reports_controller_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Api::V1::ReportsController, type: :controller do end describe 'POST #create' do - let!(:admin) { Fabricate(:user, admin: true) } + let!(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } let(:scopes) { 'write:reports' } let(:status) { Fabricate(:status) } diff --git a/spec/controllers/api/v2/admin/accounts_controller_spec.rb b/spec/controllers/api/v2/admin/accounts_controller_spec.rb index 3212ddb84..2508a9e05 100644 --- a/spec/controllers/api/v2/admin/accounts_controller_spec.rb +++ b/spec/controllers/api/v2/admin/accounts_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Api::V2::Admin::AccountsController, type: :controller do render_views - let(:role) { 'moderator' } + let(:role) { UserRole.find_by(name: 'Moderator') } let(:user) { Fabricate(:user, role: role) } let(:scopes) { 'admin:read admin:write' } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } @@ -22,7 +22,7 @@ RSpec.describe Api::V2::Admin::AccountsController, type: :controller do end shared_examples 'forbidden for wrong role' do |wrong_role| - let(:role) { wrong_role } + let(:role) { UserRole.find_by(name: wrong_role) } it 'returns http forbidden' do expect(response).to have_http_status(403) @@ -46,7 +46,7 @@ RSpec.describe Api::V2::Admin::AccountsController, type: :controller do end it_behaves_like 'forbidden for wrong scope', 'write:statuses' - it_behaves_like 'forbidden for wrong role', 'user' + it_behaves_like 'forbidden for wrong role', '' [ [{ status: 'active', origin: 'local', permissions: 'staff' }, [:admin_account]], diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 53e163d49..1b002e01c 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -183,70 +183,6 @@ describe ApplicationController, type: :controller do end end - describe 'require_admin!' do - controller do - before_action :require_admin! - - def success - head 200 - end - end - - before do - routes.draw { get 'success' => 'anonymous#success' } - end - - it 'returns a 403 if current user is not admin' do - sign_in(Fabricate(:user, admin: false)) - get 'success' - expect(response).to have_http_status(403) - end - - it 'returns a 403 if current user is only a moderator' do - sign_in(Fabricate(:user, moderator: true)) - get 'success' - expect(response).to have_http_status(403) - end - - it 'does nothing if current user is admin' do - sign_in(Fabricate(:user, admin: true)) - get 'success' - expect(response).to have_http_status(200) - end - end - - describe 'require_staff!' do - controller do - before_action :require_staff! - - def success - head 200 - end - end - - before do - routes.draw { get 'success' => 'anonymous#success' } - end - - it 'returns a 403 if current user is not admin or moderator' do - sign_in(Fabricate(:user, admin: false, moderator: false)) - get 'success' - expect(response).to have_http_status(403) - end - - it 'does nothing if current user is moderator' do - sign_in(Fabricate(:user, moderator: true)) - get 'success' - expect(response).to have_http_status(200) - end - - it 'does nothing if current user is admin' do - sign_in(Fabricate(:user, admin: true)) - get 'success' - expect(response).to have_http_status(200) - end - end - describe 'forbidden' do controller do def route_forbidden diff --git a/spec/controllers/disputes/appeals_controller_spec.rb b/spec/controllers/disputes/appeals_controller_spec.rb index faa571fc9..90f222f49 100644 --- a/spec/controllers/disputes/appeals_controller_spec.rb +++ b/spec/controllers/disputes/appeals_controller_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Disputes::AppealsController, type: :controller do before { sign_in current_user, scope: :user } - let!(:admin) { Fabricate(:user, admin: true) } + let!(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } describe '#create' do let(:current_user) { Fabricate(:user) } diff --git a/spec/controllers/invites_controller_spec.rb b/spec/controllers/invites_controller_spec.rb index 76e617e6b..23b98fb12 100644 --- a/spec/controllers/invites_controller_spec.rb +++ b/spec/controllers/invites_controller_spec.rb @@ -7,30 +7,30 @@ describe InvitesController do sign_in user end - around do |example| - min_invite_role = Setting.min_invite_role - example.run - Setting.min_invite_role = min_invite_role - end - describe 'GET #index' do subject { get :index } - let(:user) { Fabricate(:user, moderator: false, admin: false) } + let(:user) { Fabricate(:user) } let!(:invite) { Fabricate(:invite, user: user) } - context 'when user is a staff' do + context 'when everyone can invite' do + before do + UserRole.everyone.update(permissions: UserRole.everyone.permissions | UserRole::FLAGS[:invite_users]) + end + it 'renders index page' do - Setting.min_invite_role = 'user' expect(subject).to render_template :index expect(assigns(:invites)).to include invite expect(assigns(:invites).count).to eq 1 end end - context 'when user is not a staff' do + context 'when not everyone can invite' do + before do + UserRole.everyone.update(permissions: UserRole.everyone.permissions & ~UserRole::FLAGS[:invite_users]) + end + it 'returns 403' do - Setting.min_invite_role = 'modelator' expect(subject).to have_http_status 403 end end @@ -39,8 +39,12 @@ describe InvitesController do describe 'POST #create' do subject { post :create, params: { invite: { max_uses: '10', expires_in: 1800 } } } - context 'when user is an admin' do - let(:user) { Fabricate(:user, moderator: false, admin: true) } + context 'when everyone can invite' do + let(:user) { Fabricate(:user) } + + before do + UserRole.everyone.update(permissions: UserRole.everyone.permissions | UserRole::FLAGS[:invite_users]) + end it 'succeeds to create a invite' do expect { subject }.to change { Invite.count }.by(1) @@ -49,8 +53,12 @@ describe InvitesController do end end - context 'when user is not an admin' do - let(:user) { Fabricate(:user, moderator: true, admin: false) } + context 'when not everyone can invite' do + let(:user) { Fabricate(:user) } + + before do + UserRole.everyone.update(permissions: UserRole.everyone.permissions & ~UserRole::FLAGS[:invite_users]) + end it 'returns 403' do expect(subject).to have_http_status 403 @@ -61,8 +69,8 @@ describe InvitesController do describe 'DELETE #create' do subject { delete :destroy, params: { id: invite.id } } + let(:user) { Fabricate(:user) } let!(:invite) { Fabricate(:invite, user: user, expires_at: nil) } - let(:user) { Fabricate(:user, moderator: false, admin: true) } it 'expires invite' do expect(subject).to redirect_to invites_path diff --git a/spec/fabricators/user_role_fabricator.rb b/spec/fabricators/user_role_fabricator.rb new file mode 100644 index 000000000..28f76c8c4 --- /dev/null +++ b/spec/fabricators/user_role_fabricator.rb @@ -0,0 +1,5 @@ +Fabricator(:user_role) do + name "MyString" + color "MyString" + permissions "" +end \ No newline at end of file diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index dc0ca3da3..467d41836 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -445,7 +445,7 @@ RSpec.describe Account, type: :model do it 'accepts arbitrary limits' do 2.times.each { Fabricate(:account, display_name: "Display Name") } - results = Account.search_for("display", 1) + results = Account.search_for("display", limit: 1) expect(results.size).to eq 1 end @@ -473,7 +473,7 @@ RSpec.describe Account, type: :model do ) account.follow!(match) - results = Account.advanced_search_for('A?l\i:c e', account, 10, true) + results = Account.advanced_search_for('A?l\i:c e', account, limit: 10, following: true) expect(results).to eq [match] end @@ -485,7 +485,7 @@ RSpec.describe Account, type: :model do domain: 'example.com' ) - results = Account.advanced_search_for('A?l\i:c e', account, 10, true) + results = Account.advanced_search_for('A?l\i:c e', account, limit: 10, following: true) expect(results).to eq [] end @@ -498,7 +498,7 @@ RSpec.describe Account, type: :model do suspended: true ) - results = Account.advanced_search_for('username', account, 10, true) + results = Account.advanced_search_for('username', account, limit: 10, following: true) expect(results).to eq [] end @@ -511,7 +511,7 @@ RSpec.describe Account, type: :model do match.user.update(approved: false) - results = Account.advanced_search_for('username', account, 10, true) + results = Account.advanced_search_for('username', account, limit: 10, following: true) expect(results).to eq [] end @@ -524,7 +524,7 @@ RSpec.describe Account, type: :model do match.user.update(confirmed_at: nil) - results = Account.advanced_search_for('username', account, 10, true) + results = Account.advanced_search_for('username', account, limit: 10, following: true) expect(results).to eq [] end end @@ -588,7 +588,7 @@ RSpec.describe Account, type: :model do it 'accepts arbitrary limits' do 2.times { Fabricate(:account, display_name: "Display Name") } - results = Account.advanced_search_for("display", account, 1) + results = Account.advanced_search_for("display", account, limit: 1) expect(results.size).to eq 1 end diff --git a/spec/models/admin/account_action_spec.rb b/spec/models/admin/account_action_spec.rb index 809c7fc46..b6a052b76 100644 --- a/spec/models/admin/account_action_spec.rb +++ b/spec/models/admin/account_action_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Admin::AccountAction, type: :model do describe '#save!' do subject { account_action.save! } - let(:account) { Fabricate(:user, admin: true).account } + let(:account) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:target_account) { Fabricate(:account) } let(:type) { 'disable' } diff --git a/spec/models/user_role_spec.rb b/spec/models/user_role_spec.rb new file mode 100644 index 000000000..28019593e --- /dev/null +++ b/spec/models/user_role_spec.rb @@ -0,0 +1,189 @@ +require 'rails_helper' + +RSpec.describe UserRole, type: :model do + subject { described_class.create(name: 'Foo', position: 1) } + + describe '#can?' do + context 'with a single flag' do + it 'returns true if any of them are present' do + subject.permissions = UserRole::FLAGS[:manage_reports] + expect(subject.can?(:manage_reports)).to be true + end + + it 'returns false if it is not set' do + expect(subject.can?(:manage_reports)).to be false + end + end + + context 'with multiple flags' do + it 'returns true if any of them are present' do + subject.permissions = UserRole::FLAGS[:manage_users] + expect(subject.can?(:manage_reports, :manage_users)).to be true + end + + it 'returns false if none of them are present' do + expect(subject.can?(:manage_reports, :manage_users)).to be false + end + end + + context 'with an unknown flag' do + it 'raises an error' do + expect { subject.can?(:foo) }.to raise_error ArgumentError + end + end + end + + describe '#overrides?' do + it 'returns true if other role has lower position' do + expect(subject.overrides?(described_class.new(position: subject.position - 1))).to be true + end + + it 'returns true if other role is nil' do + expect(subject.overrides?(nil)).to be true + end + + it 'returns false if other role has higher position' do + expect(subject.overrides?(described_class.new(position: subject.position + 1))).to be false + end + end + + describe '#permissions_as_keys' do + before do + subject.permissions = UserRole::FLAGS[:invite_users] | UserRole::FLAGS[:view_dashboard] | UserRole::FLAGS[:manage_reports] + end + + it 'returns an array' do + expect(subject.permissions_as_keys).to match_array %w(invite_users view_dashboard manage_reports) + end + end + + describe '#permissions_as_keys=' do + let(:input) { } + + before do + subject.permissions_as_keys = input + end + + context 'with a single value' do + let(:input) { %w(manage_users) } + + it 'sets permission flags' do + expect(subject.permissions).to eq UserRole::FLAGS[:manage_users] + end + end + + context 'with multiple values' do + let(:input) { %w(manage_users manage_reports) } + + it 'sets permission flags' do + expect(subject.permissions).to eq UserRole::FLAGS[:manage_users] | UserRole::FLAGS[:manage_reports] + end + end + + context 'with an unknown value' do + let(:input) { %w(foo) } + + it 'does not set permission flags' do + expect(subject.permissions).to eq UserRole::Flags::NONE + end + end + end + + describe '#computed_permissions' do + context 'when the role is nobody' do + let(:subject) { described_class.nobody } + + it 'returns none' do + expect(subject.computed_permissions).to eq UserRole::Flags::NONE + end + end + + context 'when the role is everyone' do + let(:subject) { described_class.everyone } + + it 'returns permissions' do + expect(subject.computed_permissions).to eq subject.permissions + end + end + + context 'when role has the administrator flag' do + before do + subject.permissions = UserRole::FLAGS[:administrator] + end + + it 'returns all permissions' do + expect(subject.computed_permissions).to eq UserRole::Flags::ALL + end + end + + context do + it 'returns permissions combined with the everyone role' do + expect(subject.computed_permissions).to eq described_class.everyone.permissions + end + end + end + + describe '.everyone' do + subject { described_class.everyone } + + it 'returns a role' do + expect(subject).to be_kind_of(described_class) + end + + it 'is identified as the everyone role' do + expect(subject.everyone?).to be true + end + + it 'has default permissions' do + expect(subject.permissions).to eq UserRole::FLAGS[:invite_users] + end + + it 'has negative position' do + expect(subject.position).to eq -1 + end + end + + describe '.nobody' do + subject { described_class.nobody } + + it 'returns a role' do + expect(subject).to be_kind_of(described_class) + end + + it 'is identified as the nobody role' do + expect(subject.nobody?).to be true + end + + it 'has no permissions' do + expect(subject.permissions).to eq UserRole::Flags::NONE + end + + it 'has negative position' do + expect(subject.position).to eq -1 + end + end + + describe '#everyone?' do + it 'returns true when id is -99' do + subject.id = -99 + expect(subject.everyone?).to be true + end + + it 'returns false when id is not -99' do + subject.id = 123 + expect(subject.everyone?).to be false + end + end + + describe '#nobody?' do + it 'returns true when id is nil' do + subject.id = nil + expect(subject.nobody?).to be true + end + + it 'returns false when id is not nil' do + subject.id = 123 + expect(subject.nobody?).to be false + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 1645ab59e..a7da31e60 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -56,14 +56,6 @@ RSpec.describe User, type: :model do end end - describe 'admins' do - it 'returns an array of users who are admin' do - user_1 = Fabricate(:user, admin: false) - user_2 = Fabricate(:user, admin: true) - expect(User.admins).to match_array([user_2]) - end - end - describe 'confirmed' do it 'returns an array of users who are confirmed' do user_1 = Fabricate(:user, confirmed_at: nil) @@ -289,49 +281,6 @@ RSpec.describe User, type: :model do end end - describe '#role' do - it 'returns admin for admin' do - user = User.new(admin: true) - expect(user.role).to eq 'admin' - end - - it 'returns moderator for moderator' do - user = User.new(moderator: true) - expect(user.role).to eq 'moderator' - end - - it 'returns user otherwise' do - user = User.new - expect(user.role).to eq 'user' - end - end - - describe '#role?' do - it 'returns false when invalid role requested' do - user = User.new(admin: true) - expect(user.role?('disabled')).to be false - end - - it 'returns true when exact role match' do - user = User.new - mod = User.new(moderator: true) - admin = User.new(admin: true) - - expect(user.role?('user')).to be true - expect(mod.role?('moderator')).to be true - expect(admin.role?('admin')).to be true - end - - it 'returns true when role higher than needed' do - mod = User.new(moderator: true) - admin = User.new(admin: true) - - expect(mod.role?('user')).to be true - expect(admin.role?('user')).to be true - expect(admin.role?('moderator')).to be true - end - end - describe '#disable!' do subject(:user) { Fabricate(:user, disabled: false, current_sign_in_at: current_sign_in_at, last_sign_in_at: nil) } let(:current_sign_in_at) { Time.zone.now } @@ -420,110 +369,6 @@ RSpec.describe User, type: :model do end end - describe '#promote!' do - subject(:user) { Fabricate(:user, admin: is_admin, moderator: is_moderator) } - - before do - user.promote! - end - - context 'when user is an admin' do - let(:is_admin) { true } - - context 'when user is a moderator' do - let(:is_moderator) { true } - - it 'changes moderator filed false' do - expect(user).to be_admin - expect(user).not_to be_moderator - end - end - - context 'when user is not a moderator' do - let(:is_moderator) { false } - - it 'does not change status' do - expect(user).to be_admin - expect(user).not_to be_moderator - end - end - end - - context 'when user is not admin' do - let(:is_admin) { false } - - context 'when user is a moderator' do - let(:is_moderator) { true } - - it 'changes user into an admin' do - expect(user).to be_admin - expect(user).not_to be_moderator - end - end - - context 'when user is not a moderator' do - let(:is_moderator) { false } - - it 'changes user into a moderator' do - expect(user).not_to be_admin - expect(user).to be_moderator - end - end - end - end - - describe '#demote!' do - subject(:user) { Fabricate(:user, admin: admin, moderator: moderator) } - - before do - user.demote! - end - - context 'when user is an admin' do - let(:admin) { true } - - context 'when user is a moderator' do - let(:moderator) { true } - - it 'changes user into a moderator' do - expect(user).not_to be_admin - expect(user).to be_moderator - end - end - - context 'when user is not a moderator' do - let(:moderator) { false } - - it 'changes user into a moderator' do - expect(user).not_to be_admin - expect(user).to be_moderator - end - end - end - - context 'when user is not an admin' do - let(:admin) { false } - - context 'when user is a moderator' do - let(:moderator) { true } - - it 'changes user into a plain user' do - expect(user).not_to be_admin - expect(user).not_to be_moderator - end - end - - context 'when user is not a moderator' do - let(:moderator) { false } - - it 'does not change any fields' do - expect(user).not_to be_admin - expect(user).not_to be_moderator - end - end - end - end - describe '#active_for_authentication?' do subject { user.active_for_authentication? } let(:user) { Fabricate(:user, disabled: disabled, confirmed_at: confirmed_at) } @@ -560,4 +405,8 @@ RSpec.describe User, type: :model do end end end + + describe '.those_who_can' do + pending + end end diff --git a/spec/policies/account_moderation_note_policy_spec.rb b/spec/policies/account_moderation_note_policy_spec.rb index 39ec2008a..846747346 100644 --- a/spec/policies/account_moderation_note_policy_spec.rb +++ b/spec/policies/account_moderation_note_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe AccountModerationNotePolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :create? do @@ -31,7 +31,7 @@ RSpec.describe AccountModerationNotePolicy do context 'admin' do it 'grants to destroy' do - expect(subject).to permit(admin, AccountModerationNotePolicy) + expect(subject).to permit(admin, account_moderation_note) end end diff --git a/spec/policies/account_policy_spec.rb b/spec/policies/account_policy_spec.rb index b55eb65a7..0f23fd97e 100644 --- a/spec/policies/account_policy_spec.rb +++ b/spec/policies/account_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe AccountPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } let(:alice) { Fabricate(:account) } @@ -55,7 +55,7 @@ RSpec.describe AccountPolicy do end end - permissions :redownload?, :subscribe?, :unsubscribe? do + permissions :redownload? do context 'admin' do it 'permits' do expect(subject).to permit(admin) @@ -70,7 +70,7 @@ RSpec.describe AccountPolicy do end permissions :suspend?, :silence? do - let(:staff) { Fabricate(:user, admin: true).account } + let(:staff) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } context 'staff' do context 'record is staff' do @@ -94,7 +94,7 @@ RSpec.describe AccountPolicy do end permissions :memorialize? do - let(:other_admin) { Fabricate(:user, admin: true).account } + let(:other_admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } context 'admin' do context 'record is admin' do diff --git a/spec/policies/custom_emoji_policy_spec.rb b/spec/policies/custom_emoji_policy_spec.rb index e4f1af3c1..6a6ef6694 100644 --- a/spec/policies/custom_emoji_policy_spec.rb +++ b/spec/policies/custom_emoji_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe CustomEmojiPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :index?, :enable?, :disable? do diff --git a/spec/policies/domain_block_policy_spec.rb b/spec/policies/domain_block_policy_spec.rb index b24ed9e3a..01b97e823 100644 --- a/spec/policies/domain_block_policy_spec.rb +++ b/spec/policies/domain_block_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe DomainBlockPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :index?, :show?, :create?, :destroy? do diff --git a/spec/policies/email_domain_block_policy_spec.rb b/spec/policies/email_domain_block_policy_spec.rb index 1ff55af8e..913075c3d 100644 --- a/spec/policies/email_domain_block_policy_spec.rb +++ b/spec/policies/email_domain_block_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe EmailDomainBlockPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :index?, :create?, :destroy? do diff --git a/spec/policies/instance_policy_spec.rb b/spec/policies/instance_policy_spec.rb index 71ef1fe50..f6f51af06 100644 --- a/spec/policies/instance_policy_spec.rb +++ b/spec/policies/instance_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe InstancePolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :index?, :show?, :destroy? do diff --git a/spec/policies/invite_policy_spec.rb b/spec/policies/invite_policy_spec.rb index 122137804..01660322f 100644 --- a/spec/policies/invite_policy_spec.rb +++ b/spec/policies/invite_policy_spec.rb @@ -5,8 +5,8 @@ require 'pundit/rspec' RSpec.describe InvitePolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } - let(:john) { Fabricate(:account) } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } + let(:john) { Fabricate(:user).account } permissions :index? do context 'staff?' do @@ -17,16 +17,22 @@ RSpec.describe InvitePolicy do end permissions :create? do - context 'min_required_role?' do + context 'has privilege' do + before do + UserRole.everyone.update(permissions: UserRole::FLAGS[:invite_users]) + end + it 'permits' do - allow_any_instance_of(described_class).to receive(:min_required_role?) { true } expect(subject).to permit(john, Invite) end end - context 'not min_required_role?' do + context 'does not have privilege' do + before do + UserRole.everyone.update(permissions: UserRole::Flags::NONE) + end + it 'denies' do - allow_any_instance_of(described_class).to receive(:min_required_role?) { false } expect(subject).to_not permit(john, Invite) end end @@ -54,39 +60,15 @@ RSpec.describe InvitePolicy do end context 'not owner?' do - context 'Setting.min_invite_role == "admin"' do - before do - Setting.min_invite_role = 'admin' - end - - context 'admin?' do - it 'permits' do - expect(subject).to permit(admin, Fabricate(:invite)) - end - end - - context 'not admin?' do - it 'denies' do - expect(subject).to_not permit(john, Fabricate(:invite)) - end + context 'admin?' do + it 'permits' do + expect(subject).to permit(admin, Fabricate(:invite)) end end - context 'Setting.min_invite_role != "admin"' do - before do - Setting.min_invite_role = 'else' - end - - context 'staff?' do - it 'permits' do - expect(subject).to permit(admin, Fabricate(:invite)) - end - end - - context 'not staff?' do - it 'denies' do - expect(subject).to_not permit(john, Fabricate(:invite)) - end + context 'not admin?' do + it 'denies' do + expect(subject).to_not permit(john, Fabricate(:invite)) end end end diff --git a/spec/policies/relay_policy_spec.rb b/spec/policies/relay_policy_spec.rb index 139d945dc..2c50ba1e9 100644 --- a/spec/policies/relay_policy_spec.rb +++ b/spec/policies/relay_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe RelayPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :update? do diff --git a/spec/policies/report_note_policy_spec.rb b/spec/policies/report_note_policy_spec.rb index c34f99b71..99f5ffb8e 100644 --- a/spec/policies/report_note_policy_spec.rb +++ b/spec/policies/report_note_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe ReportNotePolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :create? do @@ -25,7 +25,8 @@ RSpec.describe ReportNotePolicy do permissions :destroy? do context 'admin?' do it 'permit' do - expect(subject).to permit(admin, ReportNote) + report_note = Fabricate(:report_note, account: john) + expect(subject).to permit(admin, report_note) end end diff --git a/spec/policies/report_policy_spec.rb b/spec/policies/report_policy_spec.rb index 84c366d7f..8b005d8dd 100644 --- a/spec/policies/report_policy_spec.rb +++ b/spec/policies/report_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe ReportPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :update?, :index?, :show? do diff --git a/spec/policies/settings_policy_spec.rb b/spec/policies/settings_policy_spec.rb index 3fa183c50..e16ee51a4 100644 --- a/spec/policies/settings_policy_spec.rb +++ b/spec/policies/settings_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe SettingsPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :update?, :show? do diff --git a/spec/policies/status_policy_spec.rb b/spec/policies/status_policy_spec.rb index 28b808ee2..205ecd720 100644 --- a/spec/policies/status_policy_spec.rb +++ b/spec/policies/status_policy_spec.rb @@ -6,7 +6,7 @@ require 'pundit/rspec' RSpec.describe StatusPolicy, type: :model do subject { described_class } - let(:admin) { Fabricate(:user, admin: true) } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')) } let(:alice) { Fabricate(:account, username: 'alice') } let(:bob) { Fabricate(:account, username: 'bob') } let(:status) { Fabricate(:status, account: alice) } diff --git a/spec/policies/tag_policy_spec.rb b/spec/policies/tag_policy_spec.rb index 256e6786a..9be7140fc 100644 --- a/spec/policies/tag_policy_spec.rb +++ b/spec/policies/tag_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe TagPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :index?, :show?, :update? do diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index 731c041d1..ff0916674 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -5,7 +5,7 @@ require 'pundit/rspec' RSpec.describe UserPolicy do let(:subject) { described_class } - let(:admin) { Fabricate(:user, admin: true).account } + let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } permissions :reset_password?, :change_email? do @@ -111,57 +111,4 @@ RSpec.describe UserPolicy do end end end - - permissions :promote? do - context 'admin?' do - context 'promotable?' do - it 'permits' do - expect(subject).to permit(admin, john.user) - end - end - - context '!promotable?' do - it 'denies' do - expect(subject).to_not permit(admin, admin.user) - end - end - end - - context '!admin?' do - it 'denies' do - expect(subject).to_not permit(john, User) - end - end - end - - permissions :demote? do - context 'admin?' do - context '!record.admin?' do - context 'demoteable?' do - it 'permits' do - john.user.update(moderator: true) - expect(subject).to permit(admin, john.user) - end - end - - context '!demoteable?' do - it 'denies' do - expect(subject).to_not permit(admin, john.user) - end - end - end - - context 'record.admin?' do - it 'denies' do - expect(subject).to_not permit(admin, admin.user) - end - end - end - - context '!admin?' do - it 'denies' do - expect(subject).to_not permit(john, User) - end - end - end end -- cgit From 9094c2f52c24e1c00b594e7c11cd00e4a07eb431 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 5 Jul 2022 12:00:27 +0200 Subject: Fix tests --- spec/controllers/admin/domain_allows_controller_spec.rb | 2 +- spec/controllers/admin/export_domain_allows_controller_spec.rb | 2 +- spec/controllers/admin/export_domain_blocks_controller_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/spec/controllers/admin/domain_allows_controller_spec.rb b/spec/controllers/admin/domain_allows_controller_spec.rb index 8bacdd3e4..6c4e67787 100644 --- a/spec/controllers/admin/domain_allows_controller_spec.rb +++ b/spec/controllers/admin/domain_allows_controller_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Admin::DomainAllowsController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'GET #new' do diff --git a/spec/controllers/admin/export_domain_allows_controller_spec.rb b/spec/controllers/admin/export_domain_allows_controller_spec.rb index f6275c2d6..1e1a5ae7d 100644 --- a/spec/controllers/admin/export_domain_allows_controller_spec.rb +++ b/spec/controllers/admin/export_domain_allows_controller_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Admin::ExportDomainAllowsController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'GET #export' do diff --git a/spec/controllers/admin/export_domain_blocks_controller_spec.rb b/spec/controllers/admin/export_domain_blocks_controller_spec.rb index 0493df859..8697e0c21 100644 --- a/spec/controllers/admin/export_domain_blocks_controller_spec.rb +++ b/spec/controllers/admin/export_domain_blocks_controller_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Admin::ExportDomainBlocksController, type: :controller do render_views before do - sign_in Fabricate(:user, admin: true), scope: :user + sign_in Fabricate(:user, role: UserRole.find_by(name: 'Admin')), scope: :user end describe 'GET #export' do -- cgit From e7aa2be828f6a632dadd5c41e2364cea91ddbb2c Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 13 Jul 2022 15:03:28 +0200 Subject: Change how hashtags are normalized (#18795) * Change how hashtags are normalized * Fix tests --- app/controllers/admin/tags_controller.rb | 4 ++- app/controllers/api/v1/featured_tags_controller.rb | 4 +-- .../settings/featured_tags_controller.rb | 1 - app/javascript/mastodon/actions/compose.js | 15 +++++++++- app/lib/ascii_folding.rb | 10 +++++++ app/lib/hashtag_normalizer.rb | 25 +++++++++++++++++ app/models/account.rb | 2 +- app/models/custom_filter.rb | 6 ++-- app/models/custom_filter_keyword.rb | 4 +-- app/models/featured_tag.rb | 31 ++++++++++++++------- app/models/tag.rb | 20 +++++++++++--- app/serializers/activitypub/hashtag_serializer.rb | 4 +-- app/serializers/rest/featured_tag_serializer.rb | 4 +++ app/serializers/rest/tag_serializer.rb | 4 +++ app/views/accounts/show.html.haml | 2 +- app/views/accounts/show.rss.ruby | 2 +- app/views/admin/tags/show.html.haml | 4 +-- app/views/admin/trends/tags/_tag.html.haml | 2 +- app/views/admin_mailer/_new_trending_tags.text.erb | 4 +-- app/views/settings/featured_tags/index.html.haml | 2 +- app/views/tags/_og.html.haml | 4 +-- app/views/tags/show.html.haml | 6 ++-- app/views/tags/show.rss.ruby | 6 ++-- config/initializers/inflections.rb | 1 + .../20220710102457_add_display_name_to_tags.rb | 5 ++++ db/schema.rb | 3 +- spec/lib/hashtag_normalizer_spec.rb | 29 ++++++++++++++++++++ spec/models/tag_spec.rb | 8 +++--- streaming/index.js | 32 ++++++++++++++++++++-- 29 files changed, 193 insertions(+), 51 deletions(-) create mode 100644 app/lib/ascii_folding.rb create mode 100644 app/lib/hashtag_normalizer.rb create mode 100644 db/migrate/20220710102457_add_display_name_to_tags.rb create mode 100644 spec/lib/hashtag_normalizer_spec.rb (limited to 'spec') diff --git a/app/controllers/admin/tags_controller.rb b/app/controllers/admin/tags_controller.rb index 749e2f144..4f727c398 100644 --- a/app/controllers/admin/tags_controller.rb +++ b/app/controllers/admin/tags_controller.rb @@ -16,6 +16,8 @@ module Admin if @tag.update(tag_params.merge(reviewed_at: Time.now.utc)) redirect_to admin_tag_path(@tag.id), notice: I18n.t('admin.tags.updated_msg') else + @time_period = (6.days.ago.to_date...Time.now.utc.to_date) + render :show end end @@ -27,7 +29,7 @@ module Admin end def tag_params - params.require(:tag).permit(:name, :trendable, :usable, :listable) + params.require(:tag).permit(:name, :display_name, :trendable, :usable, :listable) end end end diff --git a/app/controllers/api/v1/featured_tags_controller.rb b/app/controllers/api/v1/featured_tags_controller.rb index e4e836c97..c1ead4f54 100644 --- a/app/controllers/api/v1/featured_tags_controller.rb +++ b/app/controllers/api/v1/featured_tags_controller.rb @@ -13,9 +13,7 @@ class Api::V1::FeaturedTagsController < Api::BaseController end def create - @featured_tag = current_account.featured_tags.new(featured_tag_params) - @featured_tag.reset_data - @featured_tag.save! + @featured_tag = current_account.featured_tags.create!(featured_tag_params) render json: @featured_tag, serializer: REST::FeaturedTagSerializer end diff --git a/app/controllers/settings/featured_tags_controller.rb b/app/controllers/settings/featured_tags_controller.rb index e805527d0..aadff7c83 100644 --- a/app/controllers/settings/featured_tags_controller.rb +++ b/app/controllers/settings/featured_tags_controller.rb @@ -11,7 +11,6 @@ class Settings::FeaturedTagsController < Settings::BaseController def create @featured_tag = current_account.featured_tags.new(featured_tag_params) - @featured_tag.reset_data if @featured_tag.save redirect_to settings_featured_tags_path diff --git a/app/javascript/mastodon/actions/compose.js b/app/javascript/mastodon/actions/compose.js index bd4c1d002..32d8c229e 100644 --- a/app/javascript/mastodon/actions/compose.js +++ b/app/javascript/mastodon/actions/compose.js @@ -606,7 +606,20 @@ function insertIntoTagHistory(recognizedTags, text) { const state = getState(); const oldHistory = state.getIn(['compose', 'tagHistory']); const me = state.getIn(['meta', 'me']); - const names = recognizedTags.map(tag => text.match(new RegExp(`#${tag.name}`, 'i'))[0].slice(1)); + + // FIXME: Matching input hashtags with recognized hashtags has become more + // complicated because of new normalization rules, it's no longer just + // a case sensitivity issue + const names = recognizedTags.map(tag => { + const matches = text.match(new RegExp(`#${tag.name}`, 'i')); + + if (matches && matches.length > 0) { + return matches[0].slice(1); + } else { + return tag.name; + } + }); + const intersectedOldHistory = oldHistory.filter(name => names.findIndex(newName => newName.toLowerCase() === name.toLowerCase()) === -1); names.push(...intersectedOldHistory.toJS()); diff --git a/app/lib/ascii_folding.rb b/app/lib/ascii_folding.rb new file mode 100644 index 000000000..1798d3d0e --- /dev/null +++ b/app/lib/ascii_folding.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class ASCIIFolding + NON_ASCII_CHARS = 'ÀÁÂÃÄÅàáâãäåĀāĂ㥹ÇçĆćĈĉĊċČčÐðĎďĐđÈÉÊËèéêëĒēĔĕĖėĘęĚěĜĝĞğĠġĢģĤĥĦħÌÍÎÏìíîïĨĩĪīĬĭĮįİıĴĵĶķĸĹĺĻļĽľĿŀŁłÑñŃńŅņŇňʼnŊŋÒÓÔÕÖØòóôõöøŌōŎŏŐőŔŕŖŗŘřŚśŜŝŞşŠšſŢţŤťŦŧÙÚÛÜùúûüŨũŪūŬŭŮůŰűŲųŴŵÝýÿŶŷŸŹźŻżŽž' + EQUIVALENT_ASCII_CHARS = 'AAAAAAaaaaaaAaAaAaCcCcCcCcCcDdDdDdEEEEeeeeEeEeEeEeEeGgGgGgGgHhHhIIIIiiiiIiIiIiIiIiJjKkkLlLlLlLlLlNnNnNnNnnNnOOOOOOooooooOoOoOoRrRrRrSsSsSsSssTtTtTtUUUUuuuuUuUuUuUuUuUuWwYyyYyYZzZzZz' + + def fold(str) + str.tr(NON_ASCII_CHARS, EQUIVALENT_ASCII_CHARS) + end +end diff --git a/app/lib/hashtag_normalizer.rb b/app/lib/hashtag_normalizer.rb new file mode 100644 index 000000000..c1f99e163 --- /dev/null +++ b/app/lib/hashtag_normalizer.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class HashtagNormalizer + def normalize(str) + remove_invalid_characters(ascii_folding(lowercase(cjk_width(str)))) + end + + private + + def remove_invalid_characters(str) + str.gsub(/[^[:alnum:]#{Tag::HASHTAG_SEPARATORS}]/, '') + end + + def ascii_folding(str) + ASCIIFolding.new.fold(str) + end + + def lowercase(str) + str.mb_chars.downcase.to_s + end + + def cjk_width(str) + str.unicode_normalize(:nfkc) + end +end diff --git a/app/models/account.rb b/app/models/account.rb index 628692d22..fe77eaec4 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -62,7 +62,7 @@ class Account < ApplicationRecord ) USERNAME_RE = /[a-z0-9_]+([a-z0-9_\.-]+[a-z0-9_]+)?/i - MENTION_RE = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[[:word:]\.\-]+[[:word:]]+)?)/i + MENTION_RE = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[[:alnum:]\.\-]+[[:alnum:]]+)?)/i URL_PREFIX_RE = /\Ahttp(s?):\/\/[^\/]+/ include Attachmentable diff --git a/app/models/custom_filter.rb b/app/models/custom_filter.rb index e98ed7df9..985eab125 100644 --- a/app/models/custom_filter.rb +++ b/app/models/custom_filter.rb @@ -3,14 +3,14 @@ # # Table name: custom_filters # -# id :bigint not null, primary key -# account_id :bigint +# id :bigint(8) not null, primary key +# account_id :bigint(8) # expires_at :datetime # phrase :text default(""), not null # context :string default([]), not null, is an Array # created_at :datetime not null # updated_at :datetime not null -# action :integer default(0), not null +# action :integer default("warn"), not null # class CustomFilter < ApplicationRecord diff --git a/app/models/custom_filter_keyword.rb b/app/models/custom_filter_keyword.rb index bf5c55746..e0d0289ae 100644 --- a/app/models/custom_filter_keyword.rb +++ b/app/models/custom_filter_keyword.rb @@ -3,8 +3,8 @@ # # Table name: custom_filter_keywords # -# id :bigint not null, primary key -# custom_filter_id :bigint not null +# id :bigint(8) not null, primary key +# custom_filter_id :bigint(8) not null # keyword :text default(""), not null # whole_word :boolean default(TRUE), not null # created_at :datetime not null diff --git a/app/models/featured_tag.rb b/app/models/featured_tag.rb index 74d62e777..c9c285bfa 100644 --- a/app/models/featured_tag.rb +++ b/app/models/featured_tag.rb @@ -13,17 +13,19 @@ # class FeaturedTag < ApplicationRecord - belongs_to :account, inverse_of: :featured_tags, required: true - belongs_to :tag, inverse_of: :featured_tags, required: true + belongs_to :account, inverse_of: :featured_tags + belongs_to :tag, inverse_of: :featured_tags, optional: true # Set after validation - delegate :name, to: :tag, allow_nil: true - - validates_associated :tag, on: :create - validates :name, presence: true, on: :create + validate :validate_tag_name, on: :create validate :validate_featured_tags_limit, on: :create - def name=(str) - self.tag = Tag.find_or_create_by_names(str.strip)&.first + before_create :set_tag + before_create :reset_data + + attr_writer :name + + def name + tag_id.present? ? tag.name : @name end def increment(timestamp) @@ -34,14 +36,23 @@ class FeaturedTag < ApplicationRecord update(statuses_count: [0, statuses_count - 1].max, last_status_at: account.statuses.where(visibility: %i(public unlisted)).tagged_with(tag).where.not(id: deleted_status_id).select(:created_at).first&.created_at) end + private + + def set_tag + self.tag = Tag.find_or_create_by_names(@name)&.first + end + def reset_data self.statuses_count = account.statuses.where(visibility: %i(public unlisted)).tagged_with(tag).count self.last_status_at = account.statuses.where(visibility: %i(public unlisted)).tagged_with(tag).select(:created_at).first&.created_at end - private - def validate_featured_tags_limit errors.add(:base, I18n.t('featured_tags.errors.limit')) if account.featured_tags.count >= 10 end + + def validate_tag_name + errors.add(:name, :blank) if @name.blank? + errors.add(:name, :invalid) unless @name.match?(/\A(#{Tag::HASHTAG_NAME_RE})\z/i) + end end diff --git a/app/models/tag.rb b/app/models/tag.rb index a64042614..f078007f2 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -15,6 +15,7 @@ # last_status_at :datetime # max_score :float # max_score_at :datetime +# display_name :string # class Tag < ApplicationRecord @@ -24,11 +25,12 @@ class Tag < ApplicationRecord has_many :featured_tags, dependent: :destroy, inverse_of: :tag HASHTAG_SEPARATORS = "_\u00B7\u200c" - HASHTAG_NAME_RE = "([[:word:]_][[:word:]#{HASHTAG_SEPARATORS}]*[[:alpha:]#{HASHTAG_SEPARATORS}][[:word:]#{HASHTAG_SEPARATORS}]*[[:word:]_])|([[:word:]_]*[[:alpha:]][[:word:]_]*)" + HASHTAG_NAME_RE = "([[:alnum:]_][[:alnum:]#{HASHTAG_SEPARATORS}]*[[:alpha:]#{HASHTAG_SEPARATORS}][[:alnum:]#{HASHTAG_SEPARATORS}]*[[:alnum:]_])|([[:alnum:]_]*[[:alpha:]][[:alnum:]_]*)" HASHTAG_RE = /(?:^|[^\/\)\w])#(#{HASHTAG_NAME_RE})/i validates :name, presence: true, format: { with: /\A(#{HASHTAG_NAME_RE})\z/i } validate :validate_name_change, if: -> { !new_record? && name_changed? } + validate :validate_display_name_change, if: -> { !new_record? && display_name_changed? } scope :reviewed, -> { where.not(reviewed_at: nil) } scope :unreviewed, -> { where(reviewed_at: nil) } @@ -46,6 +48,10 @@ class Tag < ApplicationRecord name end + def display_name + attributes['display_name'] || name + end + def usable boolean_with_default('usable', true) end @@ -90,8 +96,10 @@ class Tag < ApplicationRecord class << self def find_or_create_by_names(name_or_names) - Array(name_or_names).map(&method(:normalize)).uniq { |str| str.mb_chars.downcase.to_s }.map do |normalized_name| - tag = matching_name(normalized_name).first || create(name: normalized_name) + names = Array(name_or_names).map { |str| [normalize(str), str] }.uniq(&:first) + + names.map do |(normalized_name, display_name)| + tag = matching_name(normalized_name).first || create(name: normalized_name, display_name: display_name) yield tag if block_given? @@ -129,7 +137,7 @@ class Tag < ApplicationRecord end def normalize(str) - str.gsub(/\A#/, '') + HashtagNormalizer.new.normalize(str) end end @@ -138,4 +146,8 @@ class Tag < ApplicationRecord def validate_name_change errors.add(:name, I18n.t('tags.does_not_match_previous_name')) unless name_was.mb_chars.casecmp(name.mb_chars).zero? end + + def validate_display_name_change + errors.add(:display_name, I18n.t('tags.does_not_match_previous_name')) unless HashtagNormalizer.new.normalize(display_name).casecmp(name.mb_chars).zero? + end end diff --git a/app/serializers/activitypub/hashtag_serializer.rb b/app/serializers/activitypub/hashtag_serializer.rb index 1a56e4dfe..90929c57f 100644 --- a/app/serializers/activitypub/hashtag_serializer.rb +++ b/app/serializers/activitypub/hashtag_serializer.rb @@ -10,11 +10,11 @@ class ActivityPub::HashtagSerializer < ActivityPub::Serializer end def name - "##{object.name}" + "##{object.display_name}" end def href - if object.class.name == 'FeaturedTag' + if object.instance_of?(FeaturedTag) short_account_tag_url(object.account, object.tag) else tag_url(object) diff --git a/app/serializers/rest/featured_tag_serializer.rb b/app/serializers/rest/featured_tag_serializer.rb index 96adcc7d0..8abcd9b90 100644 --- a/app/serializers/rest/featured_tag_serializer.rb +++ b/app/serializers/rest/featured_tag_serializer.rb @@ -12,4 +12,8 @@ class REST::FeaturedTagSerializer < ActiveModel::Serializer def url short_account_tag_url(object.account, object.tag) end + + def name + object.display_name + end end diff --git a/app/serializers/rest/tag_serializer.rb b/app/serializers/rest/tag_serializer.rb index 74aa571a4..52bfaa4ce 100644 --- a/app/serializers/rest/tag_serializer.rb +++ b/app/serializers/rest/tag_serializer.rb @@ -8,4 +8,8 @@ class REST::TagSerializer < ActiveModel::Serializer def url tag_url(object) end + + def name + object.display_name + end end diff --git a/app/views/accounts/show.html.haml b/app/views/accounts/show.html.haml index 72e9c6611..7fa688bd3 100644 --- a/app/views/accounts/show.html.haml +++ b/app/views/accounts/show.html.haml @@ -75,7 +75,7 @@ = link_to short_account_tag_path(@account, featured_tag.tag) do %h4 = fa_icon 'hashtag' - = featured_tag.name + = featured_tag.display_name %small - if featured_tag.last_status_at.nil? = t('accounts.nothing_here') diff --git a/app/views/accounts/show.rss.ruby b/app/views/accounts/show.rss.ruby index fd45a8b2b..34e29d483 100644 --- a/app/views/accounts/show.rss.ruby +++ b/app/views/accounts/show.rss.ruby @@ -28,7 +28,7 @@ RSS::Builder.build do |doc| end status.tags.each do |tag| - item.category(tag.name) + item.category(tag.display_name) end end end diff --git a/app/views/admin/tags/show.html.haml b/app/views/admin/tags/show.html.haml index fd9acce4a..104190b58 100644 --- a/app/views/admin/tags/show.html.haml +++ b/app/views/admin/tags/show.html.haml @@ -2,7 +2,7 @@ = javascript_pack_tag 'admin', async: true, crossorigin: 'anonymous' - content_for :page_title do - = "##{@tag.name}" + = "##{@tag.display_name}" - if current_user.can?(:view_dashboard) - content_for :heading_actions do @@ -53,7 +53,7 @@ = render 'shared/error_messages', object: @tag .fields-group - = f.input :name, wrapper: :with_block_label + = f.input :display_name, wrapper: :with_block_label .fields-group = f.input :usable, as: :boolean, wrapper: :with_label diff --git a/app/views/admin/trends/tags/_tag.html.haml b/app/views/admin/trends/tags/_tag.html.haml index 7bb99b158..a30666a08 100644 --- a/app/views/admin/trends/tags/_tag.html.haml +++ b/app/views/admin/trends/tags/_tag.html.haml @@ -6,7 +6,7 @@ .pending-account__header = link_to admin_tag_path(tag.id) do = fa_icon 'hashtag' - = tag.name + = tag.display_name %br/ diff --git a/app/views/admin_mailer/_new_trending_tags.text.erb b/app/views/admin_mailer/_new_trending_tags.text.erb index cde5af4e4..363df369d 100644 --- a/app/views/admin_mailer/_new_trending_tags.text.erb +++ b/app/views/admin_mailer/_new_trending_tags.text.erb @@ -1,12 +1,12 @@ <%= raw t('admin_mailer.new_trends.new_trending_tags.title') %> <% @tags.each do |tag| %> -- #<%= tag.name %> +- #<%= tag.display_name %> <%= raw t('admin.trends.tags.usage_comparison', today: tag.history.get(Time.now.utc).accounts, yesterday: tag.history.get(Time.now.utc - 1.day).accounts) %> • <%= t('admin.trends.tags.current_score', score: Trends.tags.score(tag.id).round(2)) %> <% end %> <% if @lowest_trending_tag %> -<%= raw t('admin_mailer.new_trends.new_trending_tags.requirements', lowest_tag_name: @lowest_trending_tag.name, lowest_tag_score: Trends.tags.score(@lowest_trending_tag.id).round(2), rank: Trends.tags.options[:review_threshold]) %> +<%= raw t('admin_mailer.new_trends.new_trending_tags.requirements', lowest_tag_name: @lowest_trending_tag.display_name, lowest_tag_score: Trends.tags.score(@lowest_trending_tag.id).round(2), rank: Trends.tags.options[:review_threshold]) %> <% else %> <%= raw t('admin_mailer.new_trends.new_trending_tags.no_approved_tags') %> <% end %> diff --git a/app/views/settings/featured_tags/index.html.haml b/app/views/settings/featured_tags/index.html.haml index 65de7f8f3..5d87e2862 100644 --- a/app/views/settings/featured_tags/index.html.haml +++ b/app/views/settings/featured_tags/index.html.haml @@ -9,7 +9,7 @@ = render 'shared/error_messages', object: @featured_tag .fields-group - = f.input :name, wrapper: :with_block_label, hint: safe_join([t('simple_form.hints.featured_tag.name'), safe_join(@recently_used_tags.map { |tag| link_to("##{tag.name}", settings_featured_tags_path(featured_tag: { name: tag.name }), method: :post) }, ', ')], ' ') + = f.input :name, wrapper: :with_block_label, hint: safe_join([t('simple_form.hints.featured_tag.name'), safe_join(@recently_used_tags.map { |tag| link_to("##{tag.display_name}", settings_featured_tags_path(featured_tag: { name: tag.name }), method: :post) }, ', ')], ' ') .actions = f.button :button, t('featured_tags.add_new'), type: :submit diff --git a/app/views/tags/_og.html.haml b/app/views/tags/_og.html.haml index a7c289bcb..37f644cf2 100644 --- a/app/views/tags/_og.html.haml +++ b/app/views/tags/_og.html.haml @@ -1,6 +1,6 @@ = opengraph 'og:site_name', t('about.hosted_on', domain: site_hostname) = opengraph 'og:url', tag_url(@tag) = opengraph 'og:type', 'website' -= opengraph 'og:title', "##{@tag.name}" -= opengraph 'og:description', strip_tags(t('about.about_hashtag_html', hashtag: @tag.name)) += opengraph 'og:title', "##{@tag.display_name}" += opengraph 'og:description', strip_tags(t('about.about_hashtag_html', hashtag: @tag.display_name)) = opengraph 'twitter:card', 'summary' diff --git a/app/views/tags/show.html.haml b/app/views/tags/show.html.haml index 5cd513b32..6dfb4f9b3 100644 --- a/app/views/tags/show.html.haml +++ b/app/views/tags/show.html.haml @@ -1,5 +1,5 @@ - content_for :page_title do - = "##{@tag.name}" + = "##{@tag.display_name}" - content_for :header_tags do %meta{ name: 'robots', content: 'noindex' }/ @@ -9,8 +9,8 @@ = render 'og' .page-header - %h1= "##{@tag.name}" - %p= t('about.about_hashtag_html', hashtag: @tag.name) + %h1= "##{@tag.display_name}" + %p= t('about.about_hashtag_html', hashtag: @tag.display_name) #mastodon-timeline{ data: { props: Oj.dump(default_props.merge(hashtag: @tag.name, local: @local)) }} .notranslate#modal-container diff --git a/app/views/tags/show.rss.ruby b/app/views/tags/show.rss.ruby index 9ce71be74..8e0c2327b 100644 --- a/app/views/tags/show.rss.ruby +++ b/app/views/tags/show.rss.ruby @@ -1,6 +1,6 @@ RSS::Builder.build do |doc| - doc.title("##{@tag.name}") - doc.description(I18n.t('rss.descriptions.tag', hashtag: @tag.name)) + doc.title("##{@tag.display_name}") + doc.description(I18n.t('rss.descriptions.tag', hashtag: @tag.display_name)) doc.link(tag_url(@tag)) doc.last_build_date(@statuses.first.created_at) if @statuses.any? doc.generator("Mastodon v#{Mastodon::Version.to_s}") @@ -26,7 +26,7 @@ RSS::Builder.build do |doc| end status.tags.each do |tag| - item.category(tag.name) + item.category(tag.display_name) end end end diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb index 9bc9a54b2..3e5a55617 100644 --- a/config/initializers/inflections.rb +++ b/config/initializers/inflections.rb @@ -24,6 +24,7 @@ ActiveSupport::Inflector.inflections(:en) do |inflect| inflect.acronym 'RSS' inflect.acronym 'REST' inflect.acronym 'URL' + inflect.acronym 'ASCII' inflect.singular 'data', 'data' end diff --git a/db/migrate/20220710102457_add_display_name_to_tags.rb b/db/migrate/20220710102457_add_display_name_to_tags.rb new file mode 100644 index 000000000..aa7867645 --- /dev/null +++ b/db/migrate/20220710102457_add_display_name_to_tags.rb @@ -0,0 +1,5 @@ +class AddDisplayNameToTags < ActiveRecord::Migration[6.1] + def change + add_column :tags, :display_name, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 54966ef64..9b465b674 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_07_04_024901) do +ActiveRecord::Schema.define(version: 2022_07_10_102457) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -940,6 +940,7 @@ ActiveRecord::Schema.define(version: 2022_07_04_024901) do t.datetime "last_status_at" t.float "max_score" t.datetime "max_score_at" + t.string "display_name" t.index "lower((name)::text) text_pattern_ops", name: "index_tags_on_name_lower_btree", unique: true end diff --git a/spec/lib/hashtag_normalizer_spec.rb b/spec/lib/hashtag_normalizer_spec.rb new file mode 100644 index 000000000..fbb9f37c0 --- /dev/null +++ b/spec/lib/hashtag_normalizer_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe HashtagNormalizer do + subject { described_class.new } + + describe '#normalize' do + it 'converts full-width Latin characters into basic Latin characters' do + expect(subject.normalize('Ｓｙｎｔｈｗａｖｅ')).to eq 'synthwave' + end + + it 'converts half-width Katakana into Kana characters' do + expect(subject.normalize('ｼｰｻｲﾄﾞﾗｲﾅｰ')).to eq 'シーサイドライナー' + end + + it 'converts modified Latin characters into basic Latin characters' do + expect(subject.normalize('BLÅHAJ')).to eq 'blahaj' + end + + it 'strips out invalid characters' do + expect(subject.normalize('#foo')).to eq 'foo' + end + + it 'keeps valid characters' do + expect(subject.normalize('a·b')).to eq 'a·b' + end + end +end diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index 3949dbce5..b16f99a79 100644 --- a/spec/models/tag_spec.rb +++ b/spec/models/tag_spec.rb @@ -91,7 +91,7 @@ RSpec.describe Tag, type: :model do upcase_string = 'abcABCａｂｃＡＢＣやゆよ' downcase_string = 'abcabcａｂｃａｂｃやゆよ'; - tag = Fabricate(:tag, name: downcase_string) + tag = Fabricate(:tag, name: HashtagNormalizer.new.normalize(downcase_string)) expect(Tag.find_normalized(upcase_string)).to eq tag end end @@ -101,12 +101,12 @@ RSpec.describe Tag, type: :model do upcase_string = 'abcABCａｂｃＡＢＣやゆよ' downcase_string = 'abcabcａｂｃａｂｃやゆよ'; - tag = Fabricate(:tag, name: downcase_string) + tag = Fabricate(:tag, name: HashtagNormalizer.new.normalize(downcase_string)) expect(Tag.matches_name(upcase_string)).to eq [tag] end it 'uses the LIKE operator' do - expect(Tag.matches_name('100%abc').to_sql).to eq %q[SELECT "tags".* FROM "tags" WHERE LOWER("tags"."name") LIKE LOWER('100\\%abc%')] + expect(Tag.matches_name('100%abc').to_sql).to eq %q[SELECT "tags".* FROM "tags" WHERE LOWER("tags"."name") LIKE LOWER('100abc%')] end end @@ -115,7 +115,7 @@ RSpec.describe Tag, type: :model do upcase_string = 'abcABCａｂｃＡＢＣやゆよ' downcase_string = 'abcabcａｂｃａｂｃやゆよ'; - tag = Fabricate(:tag, name: downcase_string) + tag = Fabricate(:tag, name: HashtagNormalizer.new.normalize(downcase_string)) expect(Tag.matching_name(upcase_string)).to eq [tag] end end diff --git a/streaming/index.js b/streaming/index.js index 792ec5a44..a55181bad 100644 --- a/streaming/index.js +++ b/streaming/index.js @@ -892,6 +892,34 @@ const startWorker = async (workerId) => { return arr; }; + /** + * See app/lib/ascii_folder.rb for the canon definitions + * of these constants + */ + const NON_ASCII_CHARS = 'ÀÁÂÃÄÅàáâãäåĀāĂ㥹ÇçĆćĈĉĊċČčÐðĎďĐđÈÉÊËèéêëĒēĔĕĖėĘęĚěĜĝĞğĠġĢģĤĥĦħÌÍÎÏìíîïĨĩĪīĬĭĮįİıĴĵĶķĸĹĺĻļĽľĿŀŁłÑñŃńŅņŇňʼnŊŋÒÓÔÕÖØòóôõöøŌōŎŏŐőŔŕŖŗŘřŚśŜŝŞşŠšſŢţŤťŦŧÙÚÛÜùúûüŨũŪūŬŭŮůŰűŲųŴŵÝýÿŶŷŸŹźŻżŽž'; + const EQUIVALENT_ASCII_CHARS = 'AAAAAAaaaaaaAaAaAaCcCcCcCcCcDdDdDdEEEEeeeeEeEeEeEeEeGgGgGgGgHhHhIIIIiiiiIiIiIiIiIiJjKkkLlLlLlLlLlNnNnNnNnnNnOOOOOOooooooOoOoOoRrRrRrSsSsSsSssTtTtTtUUUUuuuuUuUuUuUuUuUuWwYyyYyYZzZzZz'; + + /** + * @param {string} str + * @return {string} + */ + const foldToASCII = str => { + const regex = new RegExp(NON_ASCII_CHARS.split('').join('|'), 'g'); + + return str.replace(regex, match => { + const index = NON_ASCII_CHARS.indexOf(match); + return EQUIVALENT_ASCII_CHARS[index]; + }); + }; + + /** + * @param {string} str + * @return {string} + */ + const normalizeHashtag = str => { + return foldToASCII(str.normalize('NFKC').toLowerCase()).replace(/[^\p{L}\p{N}_\u00b7\u200c]/gu, ''); + }; + /** * @param {any} req * @param {string} name @@ -968,7 +996,7 @@ const startWorker = async (workerId) => { reject('No tag for stream provided'); } else { resolve({ - channelIds: [`timeline:hashtag:${params.tag.toLowerCase()}`], + channelIds: [`timeline:hashtag:${normalizeHashtag(params.tag)}`], options: { needsFiltering: true }, }); } @@ -979,7 +1007,7 @@ const startWorker = async (workerId) => { reject('No tag for stream provided'); } else { resolve({ - channelIds: [`timeline:hashtag:${params.tag.toLowerCase()}:local`], + channelIds: [`timeline:hashtag:${normalizeHashtag(params.tag)}:local`], options: { needsFiltering: true }, }); } -- cgit From c3f0621a59a74d0e20e6db6170894871c48e8f0f Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 17 Jul 2022 13:49:29 +0200 Subject: Add ability to follow hashtags (#18809) --- .../api/v1/featured_tags/suggestions_controller.rb | 2 +- app/controllers/api/v1/followed_tags_controller.rb | 52 ++++++++++++++ app/controllers/api/v1/tags_controller.rb | 29 ++++++++ app/controllers/api/v1/trends/tags_controller.rb | 2 +- app/lib/feed_manager.rb | 36 ++++++---- app/models/tag.rb | 5 +- app/models/tag_follow.rb | 24 +++++++ app/presenters/tag_relationships_presenter.rb | 15 ++++ app/serializers/rest/tag_serializer.rb | 14 ++++ app/services/fan_out_on_write_service.rb | 15 +++- app/workers/feed_insert_worker.rb | 8 ++- config/routes.rb | 9 +++ db/migrate/20220714171049_create_tag_follows.rb | 12 ++++ db/schema.rb | 13 +++- .../api/v1/followed_tags_controller_spec.rb | 23 ++++++ spec/controllers/api/v1/tags_controller_spec.rb | 82 ++++++++++++++++++++++ spec/fabricators/tag_follow_fabricator.rb | 4 ++ spec/models/tag_follow_spec.rb | 4 ++ 18 files changed, 329 insertions(+), 20 deletions(-) create mode 100644 app/controllers/api/v1/followed_tags_controller.rb create mode 100644 app/controllers/api/v1/tags_controller.rb create mode 100644 app/models/tag_follow.rb create mode 100644 app/presenters/tag_relationships_presenter.rb create mode 100644 db/migrate/20220714171049_create_tag_follows.rb create mode 100644 spec/controllers/api/v1/followed_tags_controller_spec.rb create mode 100644 spec/controllers/api/v1/tags_controller_spec.rb create mode 100644 spec/fabricators/tag_follow_fabricator.rb create mode 100644 spec/models/tag_follow_spec.rb (limited to 'spec') diff --git a/app/controllers/api/v1/featured_tags/suggestions_controller.rb b/app/controllers/api/v1/featured_tags/suggestions_controller.rb index 75545d3c7..76633210a 100644 --- a/app/controllers/api/v1/featured_tags/suggestions_controller.rb +++ b/app/controllers/api/v1/featured_tags/suggestions_controller.rb @@ -6,7 +6,7 @@ class Api::V1::FeaturedTags::SuggestionsController < Api::BaseController before_action :set_recently_used_tags, only: :index def index - render json: @recently_used_tags, each_serializer: REST::TagSerializer + render json: @recently_used_tags, each_serializer: REST::TagSerializer, relationships: TagRelationshipsPresenter.new(@recently_used_tags, current_user&.account_id) end private diff --git a/app/controllers/api/v1/followed_tags_controller.rb b/app/controllers/api/v1/followed_tags_controller.rb new file mode 100644 index 000000000..f0dfd044c --- /dev/null +++ b/app/controllers/api/v1/followed_tags_controller.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +class Api::V1::FollowedTagsController < Api::BaseController + TAGS_LIMIT = 100 + + before_action -> { doorkeeper_authorize! :follow, :read, :'read:follows' }, except: :show + before_action :require_user! + before_action :set_results + + after_action :insert_pagination_headers, only: :show + + def index + render json: @results.map(&:tag), each_serializer: REST::TagSerializer, relationships: TagRelationshipsPresenter.new(@results.map(&:tag), current_user&.account_id) + end + + private + + def set_results + @results = TagFollow.where(account: current_account).joins(:tag).eager_load(:tag).to_a_paginated_by_id( + limit_param(TAGS_LIMIT), + params_slice(:max_id, :since_id, :min_id) + ) + end + + def insert_pagination_headers + set_pagination_headers(next_path, prev_path) + end + + def next_path + api_v1_followed_tags_url pagination_params(max_id: pagination_max_id) if records_continue? + end + + def prev_path + api_v1_followed_tags_url pagination_params(since_id: pagination_since_id) unless @results.empty? + end + + def pagination_max_id + @results.last.id + end + + def pagination_since_id + @results.first.id + end + + def records_continue? + @results.size == limit_param(TAG_LIMIT) + end + + def pagination_params(core_params) + params.slice(:limit).permit(:limit).merge(core_params) + end +end diff --git a/app/controllers/api/v1/tags_controller.rb b/app/controllers/api/v1/tags_controller.rb new file mode 100644 index 000000000..d45015ff5 --- /dev/null +++ b/app/controllers/api/v1/tags_controller.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class Api::V1::TagsController < Api::BaseController + before_action -> { doorkeeper_authorize! :follow, :write, :'write:follows' }, except: :show + before_action :require_user!, except: :show + before_action :set_or_create_tag + + override_rate_limit_headers :follow, family: :follows + + def show + render json: @tag, serializer: REST::TagSerializer + end + + def follow + TagFollow.create!(tag: @tag, account: current_account, rate_limit: true) + render json: @tag, serializer: REST::TagSerializer + end + + def unfollow + TagFollow.find_by(account: current_account, tag: @tag)&.destroy! + render json: @tag, serializer: REST::TagSerializer + end + + private + + def set_or_create_tag + @tag = Tag.find_normalized(params[:id]) || Tag.new(name: Tag.normalize(params[:id]), display_name: params[:id]) + end +end diff --git a/app/controllers/api/v1/trends/tags_controller.rb b/app/controllers/api/v1/trends/tags_controller.rb index 41f9ffac1..21adfa2a1 100644 --- a/app/controllers/api/v1/trends/tags_controller.rb +++ b/app/controllers/api/v1/trends/tags_controller.rb @@ -8,7 +8,7 @@ class Api::V1::Trends::TagsController < Api::BaseController DEFAULT_TAGS_LIMIT = 10 def index - render json: @tags, each_serializer: REST::TagSerializer + render json: @tags, each_serializer: REST::TagSerializer, relationships: TagRelationshipsPresenter.new(@tags, current_user&.account_id) end private diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index 2eb4ba2f4..145352fe8 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -45,6 +45,8 @@ class FeedManager filter_from_list?(status, receiver) || filter_from_home?(status, receiver.account_id, build_crutches(receiver.account_id, [status])) when :mentions filter_from_mentions?(status, receiver.id) + when :tags + filter_from_tags?(status, receiver.id, build_crutches(receiver.id, [status])) else false end @@ -56,7 +58,7 @@ class FeedManager # @param [Boolean] update # @return [Boolean] def push_to_home(account, status, update: false) - return false unless add_to_feed(:home, account.id, status, account.user&.aggregates_reblogs?) + return false unless add_to_feed(:home, account.id, status, aggregate_reblogs: account.user&.aggregates_reblogs?) trim(:home, account.id) PushUpdateWorker.perform_async(account.id, status.id, "timeline:#{account.id}", { 'update' => update }) if push_update_required?("timeline:#{account.id}") @@ -69,7 +71,7 @@ class FeedManager # @param [Boolean] update # @return [Boolean] def unpush_from_home(account, status, update: false) - return false unless remove_from_feed(:home, account.id, status, account.user&.aggregates_reblogs?) + return false unless remove_from_feed(:home, account.id, status, aggregate_reblogs: account.user&.aggregates_reblogs?) redis.publish("timeline:#{account.id}", Oj.dump(event: :delete, payload: status.id.to_s)) unless update true @@ -81,7 +83,7 @@ class FeedManager # @param [Boolean] update # @return [Boolean] def push_to_list(list, status, update: false) - return false if filter_from_list?(status, list) || !add_to_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?) + return false if filter_from_list?(status, list) || !add_to_feed(:list, list.id, status, aggregate_reblogs: list.account.user&.aggregates_reblogs?) trim(:list, list.id) PushUpdateWorker.perform_async(list.account_id, status.id, "timeline:list:#{list.id}", { 'update' => update }) if push_update_required?("timeline:list:#{list.id}") @@ -94,7 +96,7 @@ class FeedManager # @param [Boolean] update # @return [Boolean] def unpush_from_list(list, status, update: false) - return false unless remove_from_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?) + return false unless remove_from_feed(:list, list.id, status, aggregate_reblogs: list.account.user&.aggregates_reblogs?) redis.publish("timeline:list:#{list.id}", Oj.dump(event: :delete, payload: status.id.to_s)) unless update true @@ -120,7 +122,7 @@ class FeedManager statuses.each do |status| next if filter_from_home?(status, into_account.id, crutches) - add_to_feed(:home, into_account.id, status, aggregate) + add_to_feed(:home, into_account.id, status, aggregate_reblogs: aggregate) end trim(:home, into_account.id) @@ -146,7 +148,7 @@ class FeedManager statuses.each do |status| next if filter_from_home?(status, list.account_id, crutches) || filter_from_list?(status, list) - add_to_feed(:list, list.id, status, aggregate) + add_to_feed(:list, list.id, status, aggregate_reblogs: aggregate) end trim(:list, list.id) @@ -161,7 +163,7 @@ class FeedManager timeline_status_ids = redis.zrange(timeline_key, 0, -1) from_account.statuses.select('id, reblog_of_id').where(id: timeline_status_ids).reorder(nil).find_each do |status| - remove_from_feed(:home, into_account.id, status, into_account.user&.aggregates_reblogs?) + remove_from_feed(:home, into_account.id, status, aggregate_reblogs: into_account.user&.aggregates_reblogs?) end end @@ -174,7 +176,7 @@ class FeedManager timeline_status_ids = redis.zrange(timeline_key, 0, -1) from_account.statuses.select('id, reblog_of_id').where(id: timeline_status_ids).reorder(nil).find_each do |status| - remove_from_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?) + remove_from_feed(:list, list.id, status, aggregate_reblogs: list.account.user&.aggregates_reblogs?) end end @@ -237,7 +239,7 @@ class FeedManager timeline_key = key(:home, account.id) account.statuses.limit(limit).each do |status| - add_to_feed(:home, account.id, status, aggregate) + add_to_feed(:home, account.id, status, aggregate_reblogs: aggregate) end account.following.includes(:account_stat).find_each do |target_account| @@ -257,7 +259,7 @@ class FeedManager statuses.each do |status| next if filter_from_home?(status, account.id, crutches) - add_to_feed(:home, account.id, status, aggregate) + add_to_feed(:home, account.id, status, aggregate_reblogs: aggregate) end trim(:home, account.id) @@ -416,6 +418,16 @@ class FeedManager false end + # Check if a status should not be added to the home feed when it comes + # from a followed hashtag + # @param [Status] status + # @param [Integer] receiver_id + # @param [Hash] crutches + # @return [Boolean] + def filter_from_tags?(status, receiver_id, crutches) + receiver_id != status.account_id && (((crutches[:active_mentions][status.id] || []) + [status.account_id]).any? { |target_account_id| crutches[:blocking][target_account_id] || crutches[:muting][target_account_id] } || crutches[:blocked_by][status.account_id] || crutches[:domain_blocking][status.account.domain]) + end + # Adds a status to an account's feed, returning true if a status was # added, and false if it was not added to the feed. Note that this is # an internal helper: callers must call trim or push updates if @@ -425,7 +437,7 @@ class FeedManager # @param [Status] status # @param [Boolean] aggregate_reblogs # @return [Boolean] - def add_to_feed(timeline_type, account_id, status, aggregate_reblogs = true) + def add_to_feed(timeline_type, account_id, status, aggregate_reblogs: true) timeline_key = key(timeline_type, account_id) reblog_key = key(timeline_type, account_id, 'reblogs') @@ -473,7 +485,7 @@ class FeedManager # @param [Status] status # @param [Boolean] aggregate_reblogs # @return [Boolean] - def remove_from_feed(timeline_type, account_id, status, aggregate_reblogs = true) + def remove_from_feed(timeline_type, account_id, status, aggregate_reblogs: true) timeline_key = key(timeline_type, account_id) reblog_key = key(timeline_type, account_id, 'reblogs') diff --git a/app/models/tag.rb b/app/models/tag.rb index f078007f2..eebf3b47d 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -22,13 +22,16 @@ class Tag < ApplicationRecord has_and_belongs_to_many :statuses has_and_belongs_to_many :accounts + has_many :passive_relationships, class_name: 'TagFollow', inverse_of: :tag, dependent: :destroy has_many :featured_tags, dependent: :destroy, inverse_of: :tag + has_many :followers, through: :passive_relationships, source: :account HASHTAG_SEPARATORS = "_\u00B7\u200c" HASHTAG_NAME_RE = "([[:alnum:]_][[:alnum:]#{HASHTAG_SEPARATORS}]*[[:alpha:]#{HASHTAG_SEPARATORS}][[:alnum:]#{HASHTAG_SEPARATORS}]*[[:alnum:]_])|([[:alnum:]_]*[[:alpha:]][[:alnum:]_]*)" HASHTAG_RE = /(?:^|[^\/\)\w])#(#{HASHTAG_NAME_RE})/i validates :name, presence: true, format: { with: /\A(#{HASHTAG_NAME_RE})\z/i } + validates :display_name, format: { with: /\A(#{HASHTAG_NAME_RE})\z/i } validate :validate_name_change, if: -> { !new_record? && name_changed? } validate :validate_display_name_change, if: -> { !new_record? && display_name_changed? } @@ -99,7 +102,7 @@ class Tag < ApplicationRecord names = Array(name_or_names).map { |str| [normalize(str), str] }.uniq(&:first) names.map do |(normalized_name, display_name)| - tag = matching_name(normalized_name).first || create(name: normalized_name, display_name: display_name) + tag = matching_name(normalized_name).first || create(name: normalized_name, display_name: display_name.gsub(/[^[:alnum:]#{HASHTAG_SEPARATORS}]/, '')) yield tag if block_given? diff --git a/app/models/tag_follow.rb b/app/models/tag_follow.rb new file mode 100644 index 000000000..abe36cd17 --- /dev/null +++ b/app/models/tag_follow.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +# == Schema Information +# +# Table name: tag_follows +# +# id :bigint(8) not null, primary key +# tag_id :bigint(8) not null +# account_id :bigint(8) not null +# created_at :datetime not null +# updated_at :datetime not null +# + +class TagFollow < ApplicationRecord + include RateLimitable + include Paginable + + belongs_to :tag + belongs_to :account + + accepts_nested_attributes_for :tag + + rate_limit by: :account, family: :follows +end diff --git a/app/presenters/tag_relationships_presenter.rb b/app/presenters/tag_relationships_presenter.rb new file mode 100644 index 000000000..c3bdbaf07 --- /dev/null +++ b/app/presenters/tag_relationships_presenter.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class TagRelationshipsPresenter + attr_reader :following_map + + def initialize(tags, current_account_id = nil, **options) + @following_map = begin + if current_account_id.nil? + {} + else + TagFollow.select(:tag_id).where(tag_id: tags.map(&:id), account_id: current_account_id).each_with_object({}) { |f, h| h[f.tag_id] = true }.merge(options[:following_map] || {}) + end + end + end +end diff --git a/app/serializers/rest/tag_serializer.rb b/app/serializers/rest/tag_serializer.rb index 52bfaa4ce..7801e77d1 100644 --- a/app/serializers/rest/tag_serializer.rb +++ b/app/serializers/rest/tag_serializer.rb @@ -5,6 +5,8 @@ class REST::TagSerializer < ActiveModel::Serializer attributes :name, :url, :history + attribute :following, if: :current_user? + def url tag_url(object) end @@ -12,4 +14,16 @@ class REST::TagSerializer < ActiveModel::Serializer def name object.display_name end + + def following + if instance_options && instance_options[:relationships] + instance_options[:relationships].following_map[object.id] || false + else + TagFollow.where(tag_id: object.id, account_id: current_user.account_id).exists? + end + end + + def current_user? + !current_user.nil? + end end diff --git a/app/services/fan_out_on_write_service.rb b/app/services/fan_out_on_write_service.rb index de5c5ebe4..ce20a146e 100644 --- a/app/services/fan_out_on_write_service.rb +++ b/app/services/fan_out_on_write_service.rb @@ -16,6 +16,7 @@ class FanOutOnWriteService < BaseService check_race_condition! fan_out_to_local_recipients! + fan_out_to_public_recipients! if broadcastable? fan_out_to_public_streams! if broadcastable? end @@ -50,6 +51,10 @@ class FanOutOnWriteService < BaseService end end + def fan_out_to_public_recipients! + deliver_to_hashtag_followers! + end + def fan_out_to_public_streams! broadcast_to_hashtag_streams! broadcast_to_public_streams! @@ -83,6 +88,14 @@ class FanOutOnWriteService < BaseService end end + def deliver_to_hashtag_followers! + TagFollow.where(tag_id: @status.tags.map(&:id)).select(:id, :account_id).reorder(nil).find_in_batches do |follows| + FeedInsertWorker.push_bulk(follows) do |follow| + [@status.id, follow.account_id, 'tags', { 'update' => update? }] + end + end + end + def deliver_to_lists! @account.lists_for_local_distribution.select(:id).reorder(nil).find_in_batches do |lists| FeedInsertWorker.push_bulk(lists) do |list| @@ -100,7 +113,7 @@ class FanOutOnWriteService < BaseService end def broadcast_to_hashtag_streams! - @status.tags.pluck(:name).each do |hashtag| + @status.tags.map(&:name).each do |hashtag| redis.publish("timeline:hashtag:#{hashtag.mb_chars.downcase}", anonymous_payload) redis.publish("timeline:hashtag:#{hashtag.mb_chars.downcase}:local", anonymous_payload) if @status.local? end diff --git a/app/workers/feed_insert_worker.rb b/app/workers/feed_insert_worker.rb index 40bc9cb6e..758cebd4b 100644 --- a/app/workers/feed_insert_worker.rb +++ b/app/workers/feed_insert_worker.rb @@ -9,7 +9,7 @@ class FeedInsertWorker @options = options.symbolize_keys case @type - when :home + when :home, :tags @follower = Account.find(id) when :list @list = List.find(id) @@ -36,6 +36,8 @@ class FeedInsertWorker case @type when :home FeedManager.instance.filter?(:home, @status, @follower) + when :tags + FeedManager.instance.filter?(:tags, @status, @follower) when :list FeedManager.instance.filter?(:list, @status, @list) end @@ -49,7 +51,7 @@ class FeedInsertWorker def perform_push case @type - when :home + when :home, :tags FeedManager.instance.push_to_home(@follower, @status, update: update?) when :list FeedManager.instance.push_to_list(@list, @status, update: update?) @@ -58,7 +60,7 @@ class FeedInsertWorker def perform_unpush case @type - when :home + when :home, :tags FeedManager.instance.unpush_from_home(@follower, @status, update: true) when :list FeedManager.instance.unpush_from_list(@list, @status, update: true) diff --git a/config/routes.rb b/config/routes.rb index 177c1cff4..7a902b1f0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -530,6 +530,15 @@ Rails.application.routes.draw do resource :note, only: :create, controller: 'accounts/notes' end + resources :tags, only: [:show], constraints: { id: /#{Tag::HASHTAG_NAME_RE}/ } do + member do + post :follow + post :unfollow + end + end + + resources :followed_tags, only: [:index] + resources :lists, only: [:index, :create, :show, :update, :destroy] do resource :accounts, only: [:show, :create, :destroy], controller: 'lists/accounts' end diff --git a/db/migrate/20220714171049_create_tag_follows.rb b/db/migrate/20220714171049_create_tag_follows.rb new file mode 100644 index 000000000..a393e90f5 --- /dev/null +++ b/db/migrate/20220714171049_create_tag_follows.rb @@ -0,0 +1,12 @@ +class CreateTagFollows < ActiveRecord::Migration[6.1] + def change + create_table :tag_follows do |t| + t.belongs_to :tag, null: false, foreign_key: { on_delete: :cascade } + t.belongs_to :account, null: false, foreign_key: { on_delete: :cascade }, index: false + + t.timestamps + end + + add_index :tag_follows, [:account_id, :tag_id], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 9b465b674..2263dc7d7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_07_10_102457) do +ActiveRecord::Schema.define(version: 2022_07_14_171049) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -928,6 +928,15 @@ ActiveRecord::Schema.define(version: 2022_07_10_102457) do t.datetime "updated_at", null: false end + create_table "tag_follows", force: :cascade do |t| + t.bigint "tag_id", null: false + t.bigint "account_id", null: false + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["account_id", "tag_id"], name: "index_tag_follows_on_account_id_and_tag_id", unique: true + t.index ["tag_id"], name: "index_tag_follows_on_tag_id" + end + create_table "tags", force: :cascade do |t| t.string "name", default: "", null: false t.datetime "created_at", null: false @@ -1167,6 +1176,8 @@ ActiveRecord::Schema.define(version: 2022_07_10_102457) do add_foreign_key "statuses", "statuses", column: "reblog_of_id", on_delete: :cascade add_foreign_key "statuses_tags", "statuses", on_delete: :cascade add_foreign_key "statuses_tags", "tags", name: "fk_3081861e21", on_delete: :cascade + add_foreign_key "tag_follows", "accounts", on_delete: :cascade + add_foreign_key "tag_follows", "tags", on_delete: :cascade add_foreign_key "tombstones", "accounts", on_delete: :cascade add_foreign_key "user_invite_requests", "users", on_delete: :cascade add_foreign_key "users", "accounts", name: "fk_50500f500d", on_delete: :cascade diff --git a/spec/controllers/api/v1/followed_tags_controller_spec.rb b/spec/controllers/api/v1/followed_tags_controller_spec.rb new file mode 100644 index 000000000..2191350ef --- /dev/null +++ b/spec/controllers/api/v1/followed_tags_controller_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +RSpec.describe Api::V1::FollowedTagsController, type: :controller do + render_views + + let(:user) { Fabricate(:user) } + let(:scopes) { 'read:follows' } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } + + before { allow(controller).to receive(:doorkeeper_token) { token } } + + describe 'GET #index' do + let!(:tag_follows) { Fabricate.times(5, :tag_follow, account: user.account) } + + before do + get :index, params: { limit: 1 } + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + end +end diff --git a/spec/controllers/api/v1/tags_controller_spec.rb b/spec/controllers/api/v1/tags_controller_spec.rb new file mode 100644 index 000000000..ac42660df --- /dev/null +++ b/spec/controllers/api/v1/tags_controller_spec.rb @@ -0,0 +1,82 @@ +require 'rails_helper' + +RSpec.describe Api::V1::TagsController, type: :controller do + render_views + + let(:user) { Fabricate(:user) } + let(:scopes) { 'write:follows' } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } + + before { allow(controller).to receive(:doorkeeper_token) { token } } + + describe 'GET #show' do + before do + get :show, params: { id: name } + end + + context 'with existing tag' do + let!(:tag) { Fabricate(:tag) } + let(:name) { tag.name } + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + end + + context 'with non-existing tag' do + let(:name) { 'hoge' } + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + end + end + + describe 'POST #follow' do + before do + post :follow, params: { id: name } + end + + context 'with existing tag' do + let!(:tag) { Fabricate(:tag) } + let(:name) { tag.name } + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'creates follow' do + expect(TagFollow.where(tag: tag, account: user.account).exists?).to be true + end + end + + context 'with non-existing tag' do + let(:name) { 'hoge' } + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'creates follow' do + expect(TagFollow.where(tag: Tag.find_by!(name: name), account: user.account).exists?).to be true + end + end + end + + describe 'POST #unfollow' do + let!(:tag) { Fabricate(:tag, name: 'foo') } + let!(:tag_follow) { Fabricate(:tag_follow, account: user.account, tag: tag) } + + before do + post :unfollow, params: { id: tag.name } + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'removes the follow' do + expect(TagFollow.where(tag: tag, account: user.account).exists?).to be false + end + end +end diff --git a/spec/fabricators/tag_follow_fabricator.rb b/spec/fabricators/tag_follow_fabricator.rb new file mode 100644 index 000000000..a2cccb07a --- /dev/null +++ b/spec/fabricators/tag_follow_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:tag_follow) do + tag + account +end diff --git a/spec/models/tag_follow_spec.rb b/spec/models/tag_follow_spec.rb new file mode 100644 index 000000000..50c04d2e4 --- /dev/null +++ b/spec/models/tag_follow_spec.rb @@ -0,0 +1,4 @@ +require 'rails_helper' + +RSpec.describe TagFollow, type: :model do +end -- cgit