From 946bb3ae9d8714550a009869355dd02cf7c2f89d Mon Sep 17 00:00:00 2001 From: Georg Gadinger Date: Sun, 19 Apr 2020 22:35:58 +0200 Subject: [PATCH] Use Rolify for admin and moderator roles --- Gemfile | 2 + Gemfile.lock | 2 + Rakefile | 52 ++++++++++--------- app/controllers/ajax/moderation_controller.rb | 20 ++++--- app/models/role.rb | 15 ++++++ app/models/user.rb | 10 +++- app/views/layouts/_profile.html.haml | 2 +- app/views/user/_actions.html.haml | 2 +- app/views/user/_modal_privileges.html.haml | 2 +- .../user/_modal_privileges_item.html.haml | 5 +- app/views/user/_profile_info.html.haml | 6 +-- app/views/user/data.html.haml | 2 +- config/initializers/rails_admin.rb | 5 +- config/initializers/rolify.rb | 12 +++++ config/routes.rb | 14 ++--- .../20200419184442_rolify_create_roles.rb | 20 +++++++ .../20200419185535_create_initial_roles.rb | 34 ++++++++++++ db/schema.rb | 20 ++++++- db/seeds.rb | 4 ++ lib/exporter.rb | 21 ++++++-- .../role_constrained_routes_spec.rb | 51 ++++++++++++++++++ 21 files changed, 246 insertions(+), 55 deletions(-) create mode 100644 app/models/role.rb create mode 100644 config/initializers/rolify.rb create mode 100644 db/migrate/20200419184442_rolify_create_roles.rb create mode 100644 db/migrate/20200419185535_create_initial_roles.rb create mode 100644 spec/integration/role_constrained_routes_spec.rb diff --git a/Gemfile b/Gemfile index 42a75408..e20ff191 100644 --- a/Gemfile +++ b/Gemfile @@ -42,6 +42,8 @@ gem 'tiny-color-rails' gem 'jquery-minicolors-rails' gem 'colorize' +gem "rolify", "~> 5.2" + source "https://rails-assets.org" do gem 'rails-assets-growl' gem 'rails-assets-jquery', '~> 2.2.0' diff --git a/Gemfile.lock b/Gemfile.lock index 44319795..c70c9ee6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -382,6 +382,7 @@ GEM responders (3.0.0) actionpack (>= 5.0) railties (>= 5.0) + rolify (5.2.0) rspec-core (3.9.1) rspec-support (~> 3.9.1) rspec-expectations (3.9.1) @@ -554,6 +555,7 @@ DEPENDENCIES rake redcarpet redis + rolify (~> 5.2) rspec-rails (~> 3.9) ruby-progressbar sanitize diff --git a/Rakefile b/Rakefile index 6ce0e0dc..ab71e59f 100644 --- a/Rakefile +++ b/Rakefile @@ -105,44 +105,48 @@ namespace :justask do end end - desc "Gives admin status to an user." + desc "Gives admin status to a user." task :admin, [:screen_name] => :environment do |t, args| - fail "screen name required" if args[:screen_name].nil? + abort "screen name required" if args[:screen_name].nil? + user = User.find_by_screen_name(args[:screen_name]) - fail "user #{args[:screen_name]} not found" if user.nil? - user.admin = true - user.save! - puts "#{user.screen_name} is now an admin." + abort "user #{args[:screen_name]} not found" if user.nil? + + user.add_role :administrator + puts "#{user.screen_name} is now an administrator." end - desc "Removes admin status from an user." + desc "Removes admin status from a user." task :deadmin, [:screen_name] => :environment do |t, args| - fail "screen name required" if args[:screen_name].nil? + abort "screen name required" if args[:screen_name].nil? + user = User.find_by_screen_name(args[:screen_name]) - fail "user #{args[:screen_name]} not found" if user.nil? - user.admin = false - user.save! - puts "#{user.screen_name} is no longer an admin." + abort "user #{args[:screen_name]} not found" if user.nil? + + user.remove_role :administrator + puts "#{user.screen_name} is no longer an administrator." end - desc "Gives moderator status to an user." + desc "Gives moderator status to a user." task :mod, [:screen_name] => :environment do |t, args| - fail "screen name required" if args[:screen_name].nil? + abort "screen name required" if args[:screen_name].nil? + user = User.find_by_screen_name(args[:screen_name]) - fail "user #{args[:screen_name]} not found" if user.nil? - user.moderator = true - user.save! - puts "#{user.screen_name} is now an moderator." + abort "user #{args[:screen_name]} not found" if user.nil? + + user.add_role :moderator + puts "#{user.screen_name} is now a moderator." end - desc "Removes moderator status from an user." + desc "Removes moderator status from a user." task :demod, [:screen_name] => :environment do |t, args| - fail "screen name required" if args[:screen_name].nil? + abort "screen name required" if args[:screen_name].nil? + user = User.find_by_screen_name(args[:screen_name]) - fail "user #{args[:screen_name]} not found" if user.nil? - user.moderator = false - user.save! - puts "#{user.screen_name} is no longer an moderator." + abort "user #{args[:screen_name]} not found" if user.nil? + + user.remove_role :moderator + puts "#{user.screen_name} is no longer a moderator." end desc "Hits an user with the banhammer." diff --git a/app/controllers/ajax/moderation_controller.rb b/app/controllers/ajax/moderation_controller.rb index 26889361..9ec82abb 100644 --- a/app/controllers/ajax/moderation_controller.rb +++ b/app/controllers/ajax/moderation_controller.rb @@ -125,9 +125,9 @@ class Ajax::ModerationController < ApplicationController unban = params[:ban] == "0" perma = params[:permaban] == "1" - buntil = DateTime.strptime params[:until], "%m/%d/%Y %I:%M %p" unless unban or perma + buntil = DateTime.strptime params[:until], "%m/%d/%Y %I:%M %p" unless unban || perma - if not unban and target.admin? + if !unban && target.has_role?(:administrator) @status = :nopriv @message = I18n.t('messages.moderation.ban.nopriv') @success = false @@ -166,7 +166,7 @@ class Ajax::ModerationController < ApplicationController @message = I18n.t('messages.moderation.privilege.nope') return unless %w(blogger supporter moderator admin contributor translator).include? params[:type].downcase - if %w(supporter moderator admin).include?(params[:type].downcase) and !current_user.admin? + if %w(supporter moderator admin).include?(params[:type].downcase) && !current_user.has_role?(:administrator) @status = :nopriv @message = I18n.t('messages.moderation.privilege.nopriv') @success = false @@ -174,7 +174,9 @@ class Ajax::ModerationController < ApplicationController end @checked = status - case params[:type].downcase + type = params[:type].downcase + target_role = {"admin" => "administrator"}.fetch(type, type).to_sym + case type when 'blogger' target_user.blogger = status when 'contributor' @@ -183,10 +185,12 @@ class Ajax::ModerationController < ApplicationController target_user.translator = status when 'supporter' target_user.supporter = status - when 'moderator' - target_user.moderator = status - when 'admin' - target_user.admin = status + when 'moderator', 'admin' + if status + target_user.add_role target_role + else + target_user.remove_role target_role + end end target_user.save! diff --git a/app/models/role.rb b/app/models/role.rb new file mode 100644 index 00000000..5db08032 --- /dev/null +++ b/app/models/role.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class Role < ApplicationRecord + has_and_belongs_to_many :users, join_table: :users_roles + + belongs_to :resource, + polymorphic: true, + optional: true + + validates :resource_type, + inclusion: { in: Rolify.resource_types }, + allow_nil: true + + scopify +end diff --git a/app/models/user.rb b/app/models/user.rb index 43e88e4f..119f8d81 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,6 +5,8 @@ class User < ApplicationRecord :recoverable, :rememberable, :trackable, :validatable, :confirmable, :authentication_keys => [:login] + rolify + # attr_accessor :login has_many :questions, dependent: :destroy @@ -183,7 +185,7 @@ class User < ApplicationRecord # @return [Boolean] is the user a moderator? def mod? - self.moderator? || self.admin? + has_role?(:moderator) || has_role?(:administrator) end # region stuff used for reporting/moderation @@ -258,4 +260,10 @@ class User < ApplicationRecord end !self.export_processing end + + # %w[admin moderator].each do |m| + # define_method(m) { raise "not allowed: #{m}" } + # define_method(m+??) { raise "not allowed: #{m}?"} + # define_method(m+?=) { |*a| raise "not allowed: #{m}="} + # end end diff --git a/app/views/layouts/_profile.html.haml b/app/views/layouts/_profile.html.haml index 9983862f..d01c7293 100644 --- a/app/views/layouts/_profile.html.haml +++ b/app/views/layouts/_profile.html.haml @@ -15,7 +15,7 @@ %i.fa.fa-fw.fa-cog = t('views.navigation.settings') %li.divider - - if current_user.admin? + - if current_user.has_role?(:administrator) %li %a{href: rails_admin_path} %i.fa.fa-fw.fa-cogs diff --git a/app/views/user/_actions.html.haml b/app/views/user/_actions.html.haml index ec19de32..7f3ae676 100644 --- a/app/views/user/_actions.html.haml +++ b/app/views/user/_actions.html.haml @@ -33,7 +33,7 @@ %a{href: '#', data: { target: "#modal-privileges", toggle: :modal }} %i.fa.fa-wrench = raw t('views.actions.privilege', user: user.screen_name) - - unless user.admin? + - unless user.has_role?(:administrator) %li %a{href: '#', data: { target: "#modal-ban", toggle: :modal }} %i.fa.fa-ban diff --git a/app/views/user/_modal_privileges.html.haml b/app/views/user/_modal_privileges.html.haml index 351e030c..f54ec705 100644 --- a/app/views/user/_modal_privileges.html.haml +++ b/app/views/user/_modal_privileges.html.haml @@ -11,7 +11,7 @@ = render 'user/modal_privileges_item', privilege: 'blogger', description: t('views.modal.privilege.blogger'), user: @user = render 'user/modal_privileges_item', privilege: 'contributor', description: t('views.modal.privilege.contributor'), user: @user = render 'user/modal_privileges_item', privilege: 'translator', description: t('views.modal.privilege.translator'), user: @user - - if current_user.admin? + - if current_user.has_role?(:administrator) = render 'user/modal_privileges_item', privilege: 'supporter', description: t('views.modal.privilege.supporter'), user: @user = render 'user/modal_privileges_item', privilege: 'moderator', description: t('views.modal.privilege.moderator'),user: @user = render 'user/modal_privileges_item', privilege: 'admin', description: t('views.modal.privilege.admin'), user: @user diff --git a/app/views/user/_modal_privileges_item.html.haml b/app/views/user/_modal_privileges_item.html.haml index 92d498dd..5164a049 100644 --- a/app/views/user/_modal_privileges_item.html.haml +++ b/app/views/user/_modal_privileges_item.html.haml @@ -1,8 +1,11 @@ - description ||= '' +- role_mapping = {"admin" => "administrator"} +- requires_role = %w[admin moderator].include?(privilege) +- checked = requires_role ? user.has_role?(role_mapping.fetch(privilege, privilege).to_sym) : user.public_send("#{privilege}?") %li.list-group-item{id: "privilege-#{privilege}"} .media .pull-left.j2-table - %input.input--center{type: :checkbox, name: 'check-your-privileges', data: { type: privilege, user: user.screen_name }, checked: user.send("#{privilege}?"), autocomplete: 'off'} + %input.input--center{type: :checkbox, name: 'check-your-privileges', data: { type: privilege, user: user.screen_name }, checked: checked, autocomplete: 'off'} .media-body .list-group-item-heading= privilege.capitalize - unless description.blank? diff --git a/app/views/user/_profile_info.html.haml b/app/views/user/_profile_info.html.haml index 57404894..0403a7e0 100644 --- a/app/views/user/_profile_info.html.haml +++ b/app/views/user/_profile_info.html.haml @@ -1,11 +1,11 @@ .panel.panel-default#profile %img.profile--avatar{src: @user.profile_picture.url(:large)} - - if user_signed_in? && current_user.admin? - - if @user.admin? + - if user_signed_in? && current_user.has_role?(:administrator) + - if @user.has_role?(:administrator) .profile--panel-badge.panel-badge-danger %i.fa.fa-flask = t 'views.user.title.admin' - - if @user.moderator? + - if @user.has_role?(:moderator) .profile--panel-badge.panel-badge-success %i.fa.fa-users = t 'views.user.title.moderator' diff --git a/app/views/user/data.html.haml b/app/views/user/data.html.haml index 9cff91a2..8dae340a 100644 --- a/app/views/user/data.html.haml +++ b/app/views/user/data.html.haml @@ -95,7 +95,7 @@ %p.data-heading Admin %p - - if current_user.admin? + - if current_user.has_role?(:administrator) %span.label.label-success %i.fa.fa-fw.fa-check - else diff --git a/config/initializers/rails_admin.rb b/config/initializers/rails_admin.rb index 2c804526..e89e849a 100644 --- a/config/initializers/rails_admin.rb +++ b/config/initializers/rails_admin.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # workaround to get pagination right if defined? WillPaginate Kaminari.configure do |config| @@ -6,12 +8,11 @@ if defined? WillPaginate end RailsAdmin.config do |config| - config.main_app_name = ['justask', 'Kontrollzentrum'] ## == Authentication == config.authenticate_with do - redirect_to main_app.root_path unless current_user.try :admin? + redirect_to main_app.root_path unless current_user&.has_role?(:administrator) end config.current_user_method(&:current_user) diff --git a/config/initializers/rolify.rb b/config/initializers/rolify.rb new file mode 100644 index 00000000..d6d8dffd --- /dev/null +++ b/config/initializers/rolify.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +Rolify.configure do |config| + # By default ORM adapter is ActiveRecord. uncomment to use mongoid + # config.use_mongoid + + # Dynamic shortcuts for User class (user.is_admin? like methods). Default is: false + # config.use_dynamic_shortcuts + + # Configuration to remove roles from database once the last resource is removed. Default is: true + config.remove_role_if_empty = false +end diff --git a/config/routes.rb b/config/routes.rb index 8176f058..e5ac3191 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -2,19 +2,19 @@ require 'sidekiq/web' Rails.application.routes.draw do start = Time.now - # Admin panel - mount RailsAdmin::Engine => '/justask_admin', as: 'rails_admin' - # Sidekiq constraints ->(req) { req.env["warden"].authenticate?(scope: :user) && - req.env['warden'].user.admin? } do + req.env["warden"].user.has_role?(:administrator) } do + # Admin panel + mount RailsAdmin::Engine => "/justask_admin", as: "rails_admin" + mount Sidekiq::Web, at: "/sidekiq" - mount PgHero::Engine, at: "/pghero", as: 'pghero' + mount PgHero::Engine, at: "/pghero", as: "pghero" end # Moderation panel - constraints ->(req) { req.env['warden'].authenticate?(scope: :user) && - (req.env['warden'].user.mod?) } do + constraints ->(req) { req.env["warden"].authenticate?(scope: :user) && + req.env["warden"].user.mod? } do match '/moderation/priority(/:user_id)', to: 'moderation#priority', via: :get, as: :moderation_priority match '/moderation/ip/:user_id', to: 'moderation#ip', via: :get, as: :moderation_ip match '/moderation(/:type)', to: 'moderation#index', via: :get, as: :moderation, defaults: {type: 'all'} diff --git a/db/migrate/20200419184442_rolify_create_roles.rb b/db/migrate/20200419184442_rolify_create_roles.rb new file mode 100644 index 00000000..23ab21f9 --- /dev/null +++ b/db/migrate/20200419184442_rolify_create_roles.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class RolifyCreateRoles < ActiveRecord::Migration[5.2] + def change + create_table(:roles) do |t| + t.string :name + t.references :resource, polymorphic: true + + t.timestamps + end + + create_table(:users_roles, id: false) do |t| + t.references :user + t.references :role + end + + add_index(:roles, %i[name resource_type resource_id]) + add_index(:users_roles, %i[user_id role_id]) + end +end diff --git a/db/migrate/20200419185535_create_initial_roles.rb b/db/migrate/20200419185535_create_initial_roles.rb new file mode 100644 index 00000000..f3f0788c --- /dev/null +++ b/db/migrate/20200419185535_create_initial_roles.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class CreateInitialRoles < ActiveRecord::Migration[5.2] + def up + %w[Administrator Moderator].each do |role| + Role.where(name: role.parameterize).first_or_create + end + + { + admin: :administrator, + moderator: :moderator + }.each do |legacy_role, new_role| + User.where(legacy_role => true).each do |u| + puts "-- migrating #{u.screen_name} (#{u.id}) from field:#{legacy_role} to role:#{new_role}" + u.add_role new_role + u.public_send("#{legacy_role}=", false) + u.save! + end + end + end + + def down + { + administrator: :admin, + moderator: :moderator + }.each do |new_role, legacy_role| + User.with_role(new_role).each do |u| + puts "-- migrating #{u.screen_name} (#{u.id}) from role:#{new_role} to field:#{legacy_role}" + u.public_send("#{legacy_role}=", true) + u.save! + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 439625e6..02d34dfe 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: 2016_01_05_165913) do +ActiveRecord::Schema.define(version: 2020_04_19_185535) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -137,6 +137,16 @@ ActiveRecord::Schema.define(version: 2016_01_05_165913) do t.string "reason" end + create_table "roles", force: :cascade do |t| + t.string "name" + t.string "resource_type" + t.bigint "resource_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["name", "resource_type", "resource_id"], name: "index_roles_on_name_and_resource_type_and_resource_id" + t.index ["resource_type", "resource_id"], name: "index_roles_on_resource_type_and_resource_id" + end + create_table "services", id: :serial, force: :cascade do |t| t.string "type", null: false t.integer "user_id", null: false @@ -270,4 +280,12 @@ ActiveRecord::Schema.define(version: 2016_01_05_165913) do t.index ["screen_name"], name: "index_users_on_screen_name", unique: true end + create_table "users_roles", id: false, force: :cascade do |t| + t.bigint "user_id" + t.bigint "role_id" + t.index ["role_id"], name: "index_users_roles_on_role_id" + t.index ["user_id", "role_id"], name: "index_users_roles_on_user_id_and_role_id" + t.index ["user_id"], name: "index_users_roles_on_user_id" + end + end diff --git a/db/seeds.rb b/db/seeds.rb index 4edb1e85..9874789c 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -5,3 +5,7 @@ # # cities = City.create([{ name: 'Chicago' }, { name: 'Copenhagen' }]) # Mayor.create(name: 'Emanuel', city: cities.first) + +%w[Administrator Moderator].each do |role| + Role.where(name: role.parameterize).first_or_create +end diff --git a/lib/exporter.rb b/lib/exporter.rb index 08f3e3c9..1086708f 100644 --- a/lib/exporter.rb +++ b/lib/exporter.rb @@ -1,9 +1,13 @@ +# frozen_string_literal: true + require 'json' require 'yaml' require 'httparty' require 'securerandom' class Exporter + EXPORT_ROLES = [:administrator, :moderator].freeze + def initialize(user) @user = user @obj = {} @@ -30,10 +34,10 @@ class Exporter private def collect_user_info - %i(admin answered_count asked_count ban_reason banned_until bio blogger comment_smiled_count commented_count + %i(answered_count asked_count ban_reason banned_until bio blogger comment_smiled_count commented_count confirmation_sent_at confirmed_at contributor created_at crop_h crop_h_h crop_h_w crop_h_x crop_h_y crop_w crop_x crop_y current_sign_in_at current_sign_in_ip display_name email follower_count friend_count - id last_sign_in_at last_sign_in_ip locale location moderator motivation_header permanently_banned + id last_sign_in_at last_sign_in_ip locale location motivation_header permanently_banned privacy_allow_anonymous_questions privacy_allow_public_timeline privacy_allow_stranger_answers privacy_show_in_search profile_header_content_type profile_header_file_name profile_header_file_size profile_header_updated_at profile_picture_content_type profile_picture_file_name profile_picture_file_size @@ -41,6 +45,10 @@ class Exporter updated_at website).each do |f| @obj[f] = @user.send f end + + EXPORT_ROLES.each do |role| + @obj[role] = @user.has_role?(role) + end end def collect_questions @@ -221,11 +229,16 @@ class Exporter def user_stub(user) uobj = {} - %i(admin answered_count asked_count bio blogger comment_smiled_count commented_count contributor created_at - display_name follower_count friend_count id location moderator motivation_header permanently_banned screen_name + %i(answered_count asked_count bio blogger comment_smiled_count commented_count contributor created_at + display_name follower_count friend_count id location motivation_header permanently_banned screen_name smiled_count supporter translator website).each do |f| uobj[f] = user.send f end + + EXPORT_ROLES.each do |role| + uobj[role] = user.has_role?(role) + end + uobj end end diff --git a/spec/integration/role_constrained_routes_spec.rb b/spec/integration/role_constrained_routes_spec.rb new file mode 100644 index 00000000..4b093876 --- /dev/null +++ b/spec/integration/role_constrained_routes_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'role-constrained routes', type: :request do + shared_examples_for 'fails to access route' do + it 'fails to access route' do + # 404 = no user found -- we have a fallback route if something could not be matched + expect(subject).to eq 404 + end + end + + shared_examples_for 'routes for' do |roles, subject_block, skip_reason: nil| + before { skip(skip_reason) } if skip_reason + + subject(&subject_block) + + context 'not signed in' do + include_examples 'fails to access route' + end + + roles.each do |role| + context "signed in user without #{role} role" do + let(:user) { FactoryBot.create(:user, password: 'test1234') } + + before(:each) do + post '/sign_in', params: { user: { login: user.email, password: user.password } } + end + + include_examples 'fails to access route' + end + + context "signed in user with #{role} role" do + let(:user) { FactoryBot.create(:user, password: 'test1234', roles: [role]) } + + before(:each) do + post '/sign_in', params: { user: { login: user.email, password: user.password } } + end + + it 'can access route' do + expect(subject).to be_in 200..299 + end + end + end + end + + it_behaves_like('routes for', [:administrator], -> { get('/justask_admin') }) + it_behaves_like('routes for', [:administrator], -> { get('/sidekiq') }) + it_behaves_like('routes for', [:administrator], -> { get('/pghero') }, skip_reason: 'PG::InFailedSqlTransaction due to 5.1 upgrade, works fine outside specs though') + it_behaves_like('routes for', %i[administrator moderator], -> { get('/moderation') }) +end