diff --git a/app/controllers/concerns/populator.rb b/app/controllers/concerns/populator.rb index 970fe0ee..214f8be7 100644 --- a/app/controllers/concerns/populator.rb +++ b/app/controllers/concerns/populator.rb @@ -74,7 +74,7 @@ module Populator def shared_user_list roles_can_appear = [] Role.where(provider: @user_domain).each do |role| - roles_can_appear << role.name if role.get_permission("can_appear_in_share_list") && role.name != "super_admin" + roles_can_appear << role.name if role.get_permission("can_appear_in_share_list") && role.priority >= 0 end initial_list = User.where.not(uid: current_user.uid) diff --git a/app/controllers/concerns/rolify.rb b/app/controllers/concerns/rolify.rb index c22691de..e2affed9 100644 --- a/app/controllers/concerns/rolify.rb +++ b/app/controllers/concerns/rolify.rb @@ -119,17 +119,32 @@ module Rolify return false if role.priority <= current_user_role.priority || role.provider != @user_domain end - # Update the roles priority including the user role - top_priority = 0 + # Get the priority of the current user's role and start with 1 higher + new_priority = [current_user_role.priority, 0].max + 1 - role_to_update.each_with_index do |id, index| - new_priority = index + [current_user_role.priority, 0].max + 1 - top_priority = new_priority - Role.where(id: id).update_all(priority: new_priority) + begin + # Save the old priorities incase something fails + old_priority = Role.where(id: role_to_update).select(:id, :priority).index_by(&:id) + + # Set all the priorities to nil to avoid unique column issues + Role.where(id: role_to_update).update_all(priority: nil) + + # Starting at the starting priority, increase by 1 every time + role_to_update.each_with_index do |id, index| + Role.find(id).update_attribute(:priority, new_priority + index) + end + + true + rescue => e + # Reset to old prorities + role_to_update.each_with_index do |id, _index| + Role.find(id).update_attribute(:priority, old_priority[id.to_i].priority) + end + + logger.error "#{current_user} failed to update role priorities: #{e}" + + false end - - user_role.priority = top_priority + 1 - user_role.save! end # Update Permissions diff --git a/app/models/role.rb b/app/models/role.rb index 9606fc6d..c6d6c598 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -38,8 +38,8 @@ class Role < ApplicationRecord send_demoted_email: true, can_edit_site_settings: true, can_manage_rooms_recordings: true, can_edit_roles: true, can_manage_users: true) Role.create(name: "pending", provider: provider, priority: -1, colour: "#17a2b8").update_all_role_permissions - Role.create(name: "denied", provider: provider, priority: -1, colour: "#343a40").update_all_role_permissions - Role.create(name: "super_admin", provider: provider, priority: -2, colour: "#cd201f") + Role.create(name: "denied", provider: provider, priority: -2, colour: "#343a40").update_all_role_permissions + Role.create(name: "super_admin", provider: provider, priority: -3, colour: "#cd201f") .update_all_role_permissions(can_create_rooms: true, send_promoted_email: true, send_demoted_email: true, can_edit_site_settings: true, can_edit_roles: true, can_manage_users: true, can_manage_rooms_recordings: true, @@ -56,8 +56,8 @@ class Role < ApplicationRecord role.priority = user_role.priority user_role.priority += 1 - role.save! user_role.save! + role.save! role end diff --git a/app/models/user.rb b/app/models/user.rb index dff85e76..bfd9f617 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -238,9 +238,9 @@ class User < ApplicationRecord end def self.all_users_highest_priority_role - User.joins("INNER JOIN (SELECT user_id, role_id, min(roles.priority) FROM users_roles " \ + User.joins("INNER JOIN (SELECT user_id, min(roles.priority) as role_priority FROM users_roles " \ "INNER JOIN roles ON users_roles.role_id = roles.id GROUP BY user_id) as a ON " \ - "a.user_id = users.id INNER JOIN roles ON roles.id = a.role_id " \ + "a.user_id = users.id INNER JOIN roles ON roles.priority = a.role_priority " \ " INNER JOIN role_permissions ON roles.id = role_permissions.role_id").distinct end diff --git a/app/views/admins/components/_menu_buttons.html.erb b/app/views/admins/components/_menu_buttons.html.erb index 6e4944bb..3af09532 100644 --- a/app/views/admins/components/_menu_buttons.html.erb +++ b/app/views/admins/components/_menu_buttons.html.erb @@ -21,13 +21,15 @@ <%= t("administrator.users.title") %> <% end %> <% end %> - <% if highest_role.get_permission("can_edit_site_settings") || highest_role.name == "super_admin" %> + <% if highest_role.get_permission("can_manage_rooms_recordings") || highest_role.name == "super_admin" %> <%= link_to admin_rooms_path, class: "list-group-item list-group-item-action dropdown-item #{"active" if active_page == "server_rooms"}" do %> <%= t("administrator.rooms.title") %> <% end %> <%= link_to admin_recordings_path, class: "list-group-item list-group-item-action dropdown-item #{"active" if active_page == "server_recordings"}" do %> <%= t("administrator.recordings.title") %> <% end %> + <% end %> + <% if highest_role.get_permission("can_edit_site_settings") || highest_role.name == "super_admin" %> <%= link_to admin_site_settings_path, class: "list-group-item list-group-item-action dropdown-item #{"active" if active_page == "site_settings"}" do %> <%= t("administrator.site_settings.title") %> <% end %> diff --git a/app/views/shared/_header.html.erb b/app/views/shared/_header.html.erb index 2b2d9c41..92d717b5 100755 --- a/app/views/shared/_header.html.erb +++ b/app/views/shared/_header.html.erb @@ -63,6 +63,10 @@ <%= link_to admins_path, class: "dropdown-item" do %> <%= t("header.dropdown.account_settings") %> <% end %> + <% elsif highest_role.get_permission("can_manage_rooms_recordings")%> + <%= link_to admin_rooms_path, class: "dropdown-item" do %> + <%= t("header.dropdown.account_settings") %> + <% end %> <% elsif highest_role.get_permission("can_edit_site_settings") %> <%= link_to admin_site_settings_path, class: "dropdown-item" do %> <%= t("header.dropdown.account_settings") %> diff --git a/db/migrate/20200130144841_change_role_priority_to_unique.rb b/db/migrate/20200130144841_change_role_priority_to_unique.rb new file mode 100644 index 00000000..ca4ee740 --- /dev/null +++ b/db/migrate/20200130144841_change_role_priority_to_unique.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class MigrationProduct < ActiveRecord::Base + self.table_name = :roles +end + +class ChangeRolePriorityToUnique < ActiveRecord::Migration[5.2] + def change + reversible do |dir| + dir.up do + MigrationProduct.where("priority < 0").where.not(name: "pending").each do |role| + role.decrement!(:priority) + end + + add_index MigrationProduct, [:priority, :provider], unique: true + end + + dir.down do + remove_index MigrationProduct, [:priority, :provider] + + MigrationProduct.where("priority < 0").where.not(name: "pending").each do |role| + role.increment!(:priority) + end + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 9b678e25..9385d04c 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: 2019_11_28_212935) do +ActiveRecord::Schema.define(version: 2020_01_30_144841) do create_table "features", force: :cascade do |t| t.integer "setting_id" @@ -58,6 +58,7 @@ ActiveRecord::Schema.define(version: 2019_11_28_212935) do t.datetime "updated_at", null: false t.index ["name", "provider"], name: "index_roles_on_name_and_provider", unique: true t.index ["name"], name: "index_roles_on_name" + t.index ["priority", "provider"], name: "index_roles_on_priority_and_provider", unique: true end create_table "rooms", force: :cascade do |t| diff --git a/spec/controllers/admins_controller_spec.rb b/spec/controllers/admins_controller_spec.rb index 0801bca0..d2e59de3 100644 --- a/spec/controllers/admins_controller_spec.rb +++ b/spec/controllers/admins_controller_spec.rb @@ -497,6 +497,7 @@ describe AdminsController, type: :controller do context "PATCH #change_role_order" do before do Role.create_default_roles("provider1") + @user.roles.delete(Role.find_by(name: "user", provider: "greenlight")) end it "should fail if user attempts to change the order of the admin or user roles" do @@ -512,35 +513,9 @@ describe AdminsController, type: :controller do end it "should fail if a user attempts to edit a role with a higher priority than their own" do - Role.create(name: "test1", priority: 1, provider: "greenlight") - new_role2 = Role.create(name: "test2", priority: 2, provider: "greenlight") + new_role3 = Role.create_new_role("test3", "provider1") + new_role2 = Role.create_new_role("test2", "provider1") new_role2.update_permission("can_edit_roles", "true") - new_role3 = Role.create(name: "test3", priority: 3, provider: "greenlight") - user_role = Role.find_by(name: "user", provider: "greenlight") - - user_role.priority = 4 - user_role.save! - - @user.roles << new_role2 - @user.save! - - @request.session[:user_id] = @user.id - - patch :change_role_order, params: { role: [new_role3.id, new_role2.id] } - - expect(flash[:alert]).to eq(I18n.t("administrator.roles.invalid_order")) - expect(response).to redirect_to admin_roles_path - end - - it "should fail if a user attempts to edit a role with a higher priority than their own" do - Role.create(name: "test1", priority: 1, provider: "greenlight") - new_role2 = Role.create(name: "test2", priority: 2, provider: "greenlight") - new_role2.update_permission("can_edit_roles", "true") - new_role3 = Role.create(name: "test3", priority: 3, provider: "greenlight") - user_role = Role.find_by(name: "user", provider: "greenlight") - - user_role.priority = 4 - user_role.save! @user.roles << new_role2 @user.save! @@ -554,10 +529,11 @@ describe AdminsController, type: :controller do end it "should update the role order" do + user_role = Role.find_by(name: "user", provider: "provider1") + user_role.update_attribute(:priority, 4) new_role1 = Role.create(name: "test1", priority: 1, provider: "provider1") new_role2 = Role.create(name: "test2", priority: 2, provider: "provider1") new_role3 = Role.create(name: "test3", priority: 3, provider: "provider1") - user_role = Role.find_by(name: "user", provider: "provider1") @request.session[:user_id] = @admin.id @@ -578,16 +554,15 @@ describe AdminsController, type: :controller do context 'POST #update_role' do before do Role.create_default_roles("provider1") + @user.roles.delete(Role.find_by(name: "user", provider: "greenlight")) end it "should fail to update a role with a lower priority than the user" do + user_role = Role.find_by(name: "user", provider: "provider1") + user_role.update_attribute(:priority, 3) new_role1 = Role.create(name: "test1", priority: 1, provider: "provider1") new_role2 = Role.create(name: "test2", priority: 2, provider: "provider1") new_role2.update_permission("can_edit_roles", "true") - user_role = Role.find_by(name: "user", provider: "greenlight") - - user_role.priority = 3 - user_role.save! @user.roles << new_role2 @user.save! @@ -601,7 +576,7 @@ describe AdminsController, type: :controller do end it "should fail to update if there is a duplicate name" do - new_role = Role.create(name: "test2", priority: 1, provider: "provider1") + new_role = Role.create(name: "test2", priority: 2, provider: "provider1") new_role.update_permission("can_edit_roles", "true") @request.session[:user_id] = @admin.id @@ -613,7 +588,7 @@ describe AdminsController, type: :controller do end it "should update role permisions" do - new_role = Role.create(name: "test2", priority: 1, provider: "provider1") + new_role = Role.create(name: "test2", priority: 2, provider: "provider1") new_role.update_permission("can_edit_roles", "true") @request.session[:user_id] = @admin.id @@ -658,7 +633,7 @@ describe AdminsController, type: :controller do end it "should successfully delete the role" do - new_role = Role.create(name: "test2", priority: 1, provider: "provider1") + new_role = Role.create(name: "test2", priority: 2, provider: "provider1") new_role.update_permission("can_edit_roles", "true") @request.session[:user_id] = @admin.id diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index c33daf74..453f981f 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -312,7 +312,7 @@ describe UsersController, type: :controller do user_role.save! - tmp_role = Role.create(name: "test", priority: -2, provider: "greenlight") + tmp_role = Role.create(name: "test", priority: -4, provider: "greenlight") params = random_valid_user_params patch :update, params: params.merge!(user_uid: user, user: { role_ids: tmp_role.id.to_s }) @@ -354,9 +354,9 @@ describe UsersController, type: :controller do @request.session[:user_id] = admin.id - tmp_role1 = Role.create(name: "test1", priority: 1, provider: "greenlight") + tmp_role1 = Role.create(name: "test1", priority: 2, provider: "greenlight") tmp_role1.update_permission("send_promoted_email", "true") - tmp_role2 = Role.create(name: "test2", priority: 2, provider: "greenlight") + tmp_role2 = Role.create(name: "test2", priority: 3, provider: "greenlight") params = random_valid_user_params params = params.merge!(user_uid: user, user: { role_ids: "#{tmp_role1.id} #{tmp_role2.id}" }) @@ -375,7 +375,7 @@ describe UsersController, type: :controller do admin.add_role :admin - tmp_role1 = Role.create(name: "test1", priority: 1, provider: "greenlight") + tmp_role1 = Role.create(name: "test1", priority: 2, provider: "greenlight") tmp_role1.update_permission("send_demoted_email", "true") user.roles << tmp_role1 user.save!