From 674735c2373725f2e19eca6d2586fd54980f2f08 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Fri, 17 Jan 2025 22:13:40 -0500 Subject: [PATCH 1/4] Remove pundit and related code from Rails --- Gemfile | 1 - Gemfile.lock | 3 - app/controllers/api/api_controller.rb | 1 - .../api/v1/current_user_controller.rb | 1 - app/controllers/api/v1/scorers_controller.rb | 7 +- app/models/concerns/permissible.rb | 245 ------------------ app/models/permission.rb | 30 --- app/models/user.rb | 4 - app/policies/application_policy.rb | 55 ---- app/policies/case_policy.rb | 52 ---- app/policies/query_policy.rb | 44 ---- app/policies/scorer_policy.rb | 48 ---- app/policies/snapshot_policy.rb | 44 ---- app/policies/team_policy.rb | 44 ---- app/policies/try_policy.rb | 44 ---- app/policies/user_policy.rb | 52 ---- app/services/permissions_evaluator.rb | 42 --- .../api/v1/current_user/show.json.jbuilder | 1 - .../20250118025829_drop_permissions_table.rb | 5 + db/schema.rb | 12 +- docs/app_structure.md | 19 -- .../api/v1/current_user_controller_test.rb | 36 --- .../api/v1/scorers_controller_test.rb | 15 ++ test/models/permission_test.rb | 66 ----- test/services/permissions_evaluator_test.rb | 48 ---- 25 files changed, 22 insertions(+), 897 deletions(-) delete mode 100644 app/models/concerns/permissible.rb delete mode 100644 app/models/permission.rb delete mode 100644 app/policies/application_policy.rb delete mode 100644 app/policies/case_policy.rb delete mode 100644 app/policies/query_policy.rb delete mode 100644 app/policies/scorer_policy.rb delete mode 100644 app/policies/snapshot_policy.rb delete mode 100644 app/policies/team_policy.rb delete mode 100644 app/policies/try_policy.rb delete mode 100644 app/policies/user_policy.rb delete mode 100644 app/services/permissions_evaluator.rb create mode 100644 db/migrate/20250118025829_drop_permissions_table.rb delete mode 100644 test/models/permission_test.rb delete mode 100644 test/services/permissions_evaluator_test.rb diff --git a/Gemfile b/Gemfile index bd596d75c..9e19475ea 100644 --- a/Gemfile +++ b/Gemfile @@ -41,7 +41,6 @@ gem 'omniauth-rails_csrf_protection' gem 'postmark-rails' gem 'prophet-rb', '~> 0.5.3' gem 'puma' -gem 'pundit' gem 'rails', '8.0.1' gem 'rails-html-sanitizer' gem 'rack-cors', '~> 2.0' diff --git a/Gemfile.lock b/Gemfile.lock index cd81fc391..d2ea75d27 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -362,8 +362,6 @@ GEM public_suffix (6.0.1) puma (6.5.0) nio4r (~> 2.0) - pundit (2.4.0) - activesupport (>= 3.0.0) raabro (1.4.0) racc (1.8.1) rack (3.1.8) @@ -591,7 +589,6 @@ DEPENDENCIES postmark-rails prophet-rb (~> 0.5.3) puma - pundit rack-cors (~> 2.0) rails (= 8.0.1) rails-controller-testing diff --git a/app/controllers/api/api_controller.rb b/app/controllers/api/api_controller.rb index eb95dcf6d..9e745cc7f 100644 --- a/app/controllers/api/api_controller.rb +++ b/app/controllers/api/api_controller.rb @@ -5,7 +5,6 @@ # rubocop:disable Rails/ApplicationController module Api class ApiController < ActionController::Base - include Pundit::Authorization include Authentication::CurrentUserManager include Authentication::CurrentCaseManager include Authentication::CurrentQueryManager diff --git a/app/controllers/api/v1/current_user_controller.rb b/app/controllers/api/v1/current_user_controller.rb index de5872a19..fac4abe08 100644 --- a/app/controllers/api/v1/current_user_controller.rb +++ b/app/controllers/api/v1/current_user_controller.rb @@ -4,7 +4,6 @@ module Api module V1 class CurrentUserController < Api::ApiController def show - @permissions = PermissionsEvaluator.new(current_user).run @user = current_user respond_with @user diff --git a/app/controllers/api/v1/scorers_controller.rb b/app/controllers/api/v1/scorers_controller.rb index 11b261c36..8d130dccc 100644 --- a/app/controllers/api/v1/scorers_controller.rb +++ b/app/controllers/api/v1/scorers_controller.rb @@ -50,12 +50,7 @@ def create # rubocop:disable Metrics/MethodLength def update - # this method could be used instead of the below @scorer.owner == current_user logic - # authorize @scorer, :update_communal? - - # the policy() call is provided by Pundit and leverages the Permissions data structures. - # using this check instead of the authorize because it raises an exception. - unless @scorer.owner == current_user || (@scorer.communal && policy(@scorer).update_communal?) + unless @scorer.owner == current_user || (@scorer.communal && current_user.administrator?) render( json: { error: 'Cannot edit a scorer you do not own', diff --git a/app/models/concerns/permissible.rb b/app/models/concerns/permissible.rb deleted file mode 100644 index 24d815f68..000000000 --- a/app/models/concerns/permissible.rb +++ /dev/null @@ -1,245 +0,0 @@ -# frozen_string_literal: true - -# rubocop:disable Metrics/ModuleLength -module Permissible - extend ActiveSupport::Concern - - PERMISSIONS = [ - { - model_type: 'case', - action: 'create', - on: { user: true }, - }, - { - model_type: 'case', - action: 'create_multi', - on: { user: true }, - }, - { - model_type: 'case', - action: 'update', - on: { user: true }, - }, - { - model_type: 'case', - action: 'read', - on: { user: true }, - }, - { - model_type: 'case', - action: 'delete', - on: { user: true }, - }, - { - model_type: 'query', - action: 'create', - on: { user: true }, - }, - { - model_type: 'query', - action: 'update', - on: { user: true }, - }, - { - model_type: 'query', - action: 'read', - on: { user: true }, - }, - { - model_type: 'query', - action: 'delete', - on: { user: true }, - }, - { - model_type: 'scorer', - action: 'create', - on: { user: true }, - }, - { - model_type: 'scorer', - action: 'update', - on: { user: true }, - }, - { - model_type: 'scorer', - action: 'update_communal', - on: { user: true }, - }, - { - model_type: 'scorer', - action: 'read', - on: { user: true }, - }, - { - model_type: 'scorer', - action: 'delete', - on: { user: true }, - }, - { - model_type: 'snapshot', - action: 'create', - on: { user: true }, - }, - { - model_type: 'snapshot', - action: 'update', - on: { user: true }, - }, - { - model_type: 'snapshot', - action: 'read', - on: { user: true }, - }, - { - model_type: 'snapshot', - action: 'delete', - on: { user: true }, - }, - { - model_type: 'team', - action: 'create', - on: { user: true }, - }, - { - model_type: 'team', - action: 'update', - on: { user: true }, - }, - { - model_type: 'team', - action: 'read', - on: { user: true }, - }, - { - model_type: 'team', - action: 'delete', - on: { user: true }, - }, - { - model_type: 'try', - action: 'create', - on: { user: true }, - }, - { - model_type: 'try', - action: 'update', - on: { user: true }, - }, - { - model_type: 'try', - action: 'read', - on: { user: true }, - }, - { - model_type: 'try', - action: 'delete', - on: { user: true }, - }, - { - model_type: 'user', - action: 'create', - on: { user: false }, - }, - { - model_type: 'user', - action: 'update', - on: { user: true }, - }, - { - model_type: 'user', - action: 'read', - on: { user: true }, - }, - { - model_type: 'user', - action: 'delete', - on: { user: false }, - } - ].freeze - - def self.grouped_permissions - PERMISSIONS.each_with_object({}) do |permission, grouped| - model_type = permission[:model_type].to_sym - grouped[model_type] ||= {} - grouped[model_type][permission[:action].to_sym] = permission[:on] - end - end - - def self.defaults - PERMISSIONS.each_with_object({}) do |permission, grouped| - model_type = permission[:model_type].to_sym - grouped[model_type] ||= {} - grouped[model_type][permission[:action].to_sym] = permission[:on][:user] - end - end - - included do - before_save :set_default_permissions - end - - def set_user_default_permissions - set_permissions level: :user - end - - def fetch_permission model_type, action - permissions.find_by( - model_type: model_type, - action: action - ) - end - - # rubocop:disable Naming/PredicateName - def has_permission? model_type, action - permissions.exists?(model_type: model_type, action: action) - end - # rubocop:enable Naming/PredicateName - - def permitted_to? model_type, action - permission = fetch_permission model_type, action - permission.on - end - - def permissions_hash - hash = {} - - permissions.each do |permission| - hash[permission.model_type.to_sym] ||= {} - hash[permission.model_type.to_sym][permission.action.to_sym] = permission.on - end - - hash - end - - private - - def set_default_permissions - set_user_default_permissions if new_record? - - true - end - - # rubocop:disable Metrics/MethodLength - def set_permissions level: :user - PERMISSIONS.each do |permission| - existing = fetch_permission permission[:model_type], permission[:action] - - if existing.present? - existing.update on: permission[:on][level] - elsif persisted? - permissions.create( - model_type: permission[:model_type], - action: permission[:action], - on: permission[:on][level] - ) - else - permissions.build( - model_type: permission[:model_type], - action: permission[:action], - on: permission[:on][level] - ) - end - end - end - # rubocop:enable Metrics/MethodLength -end -# rubocop:enable Metrics/ModuleLength diff --git a/app/models/permission.rb b/app/models/permission.rb deleted file mode 100644 index 2195cd55d..000000000 --- a/app/models/permission.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -# == Schema Information -# -# Table name: permissions -# -# id :integer not null, primary key -# action :string(255) not null -# model_type :string(255) not null -# on :boolean default(FALSE) -# created_at :datetime not null -# updated_at :datetime not null -# user_id :integer -# -# Indexes -# -# index_permissions_user_id (user_id) -# - -class Permission < ApplicationRecord - # Associations - belongs_to :user - - # Validations - validates :model_type, - presence: true - - validates :action, - presence: true -end diff --git a/app/models/user.rb b/app/models/user.rb index 4d0bbe887..853d55b1f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -104,9 +104,6 @@ class User < ApplicationRecord through: :teams, source: :scorers - has_many :permissions, - dependent: :destroy - has_many :scores, dependent: :destroy @@ -210,7 +207,6 @@ def store_raw_invitation_token # END devise hacks # Concerns - include Permissible include Profile # Scopes diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb deleted file mode 100644 index 150d6bb5a..000000000 --- a/app/policies/application_policy.rb +++ /dev/null @@ -1,55 +0,0 @@ -# frozen_string_literal: true - -class ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - false - end - - def show? - scope.exists?(id: record.id) - end - - def create? - false - end - - def new? - create? - end - - def update? - false - end - - def edit? - update? - end - - def destroy? - false - end - - def scope - Pundit.policy_scope!(user, record.class) - end - - class Scope - attr_reader :user, :scope - - def initialize user, scope - @user = user - @scope = scope - end - - def resolve - scope - end - end -end diff --git a/app/policies/case_policy.rb b/app/policies/case_policy.rb deleted file mode 100644 index 84b19e8e4..000000000 --- a/app/policies/case_policy.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -class CasePolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - true - end - - def show? - if @record.instance_of? Class - permissions[:read] - else - permissions[:read] && ( @record.is_owner?(@user) || @record.shared_with?(@user) || @record.public? ) - end - end - - def create? - if @user.cases.not_archived.count.zero? - permissions[:create] - else - permissions[:create_multi] - end - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def edit? - update? - end - - def destroy? - permissions[:delete] && @record.is_owner?(@user) - end - - private - - def permissions - @user.permissions_hash[:case] || Permissible.defaults[:case] - end -end diff --git a/app/policies/query_policy.rb b/app/policies/query_policy.rb deleted file mode 100644 index cd2115d2a..000000000 --- a/app/policies/query_policy.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -class QueryPolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - true - end - - def show? - permissions[:read] - end - - def create? - permissions[:create] - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def edit? - update? - end - - def destroy? - permissions[:delete] - end - - private - - def permissions - @user.permissions_hash[:query] || Permissible.defaults[:query] - end -end diff --git a/app/policies/scorer_policy.rb b/app/policies/scorer_policy.rb deleted file mode 100644 index 75876e954..000000000 --- a/app/policies/scorer_policy.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -class ScorerPolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - true - end - - def show? - permissions[:read] - end - - def create? - permissions[:create] - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def update_communal? - @user.administrator - end - - def edit? - update? - end - - def destroy? - permissions[:delete] - end - - private - - def permissions - @user.permissions_hash[:scorer] || Permissible.defaults[:scorer] - end -end diff --git a/app/policies/snapshot_policy.rb b/app/policies/snapshot_policy.rb deleted file mode 100644 index c0c0b65a2..000000000 --- a/app/policies/snapshot_policy.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -class SnapshotPolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - true - end - - def show? - permissions[:read] - end - - def create? - permissions[:create] - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def edit? - update? - end - - def destroy? - permissions[:delete] - end - - private - - def permissions - @user.permissions_hash[:snapshot] || Permissible.defaults[:snapshot] - end -end diff --git a/app/policies/team_policy.rb b/app/policies/team_policy.rb deleted file mode 100644 index ee463e1ac..000000000 --- a/app/policies/team_policy.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -class TeamPolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - true - end - - def show? - permissions[:read] - end - - def create? - permissions[:create] - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def edit? - update? - end - - def destroy? - permissions[:delete] - end - - private - - def permissions - @user.permissions_hash[:team] || Permissible.defaults[:team] - end -end diff --git a/app/policies/try_policy.rb b/app/policies/try_policy.rb deleted file mode 100644 index 197b2a72c..000000000 --- a/app/policies/try_policy.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -class TryPolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - true - end - - def show? - permissions[:read] - end - - def create? - permissions[:create] - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def edit? - update? - end - - def destroy? - permissions[:delete] - end - - private - - def permissions - @user.permissions_hash[:try] || Permissible.defaults[:try] - end -end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb deleted file mode 100644 index e549178ad..000000000 --- a/app/policies/user_policy.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -class UserPolicy < ApplicationPolicy - attr_reader :user, :record - - def initialize user, record - @user = user - @record = record - end - - def index? - false - end - - def show? - if @record.instance_of? Class - permissions[:read] - else - permissions[:read] && @record.id == @user.id - end - end - - def create? - permissions[:create] - end - - def new? - create? - end - - def update? - permissions[:update] && show? - end - - def edit? - update? - end - - def destroy? - if @record.instance_of? Class - permissions[:delete] - else - permissions[:delete] && @record.id == @user.id - end - end - - private - - def permissions - @user.permissions_hash[:user] || Permissible.defaults[:user] - end -end diff --git a/app/services/permissions_evaluator.rb b/app/services/permissions_evaluator.rb deleted file mode 100644 index b7420332f..000000000 --- a/app/services/permissions_evaluator.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -class PermissionsEvaluator - def initialize user - @user = user - end - - # rubocop:disable Metrics/MethodLength - def run - grouped_permissions = Permissible.grouped_permissions - permissions = @user.permissions_hash - evaled_permissions = {} - - grouped_permissions.each do |model_name, actions| - evaled_permissions[model_name] ||= {} - - model_class = model_name.to_s.capitalize - policy_class = "#{model_name.to_s.capitalize}Policy" - policy = Pundit.policy(@user, model_class.constantize) - - actions.each_key do |action| - user_value = permissions[model_name][action] if permissions[model_name] - - if policy_class.constantize.try(:send, :method_defined?, :"#{action}?") - policy_value = policy.try(:send, :"#{action}?") - end - - # Not using the shorthand || because: - # false || true == true - # and in this case we want the first value to be deciding value - # and not override it by the default values. - final_value = policy_value - final_value = user_value if final_value.nil? - - evaled_permissions[model_name][action] = final_value - end - end - - evaled_permissions - end - # rubocop:enable Metrics/MethodLength -end diff --git a/app/views/api/v1/current_user/show.json.jbuilder b/app/views/api/v1/current_user/show.json.jbuilder index b2e7c817a..a65bd95b8 100644 --- a/app/views/api/v1/current_user/show.json.jbuilder +++ b/app/views/api/v1/current_user/show.json.jbuilder @@ -2,4 +2,3 @@ json.partial! 'api/v1/users/user', user: @user json.completed_case_wizard @user.completed_case_wizard -json.permissions @permissions diff --git a/db/migrate/20250118025829_drop_permissions_table.rb b/db/migrate/20250118025829_drop_permissions_table.rb new file mode 100644 index 000000000..bda9c254e --- /dev/null +++ b/db/migrate/20250118025829_drop_permissions_table.rb @@ -0,0 +1,5 @@ +class DropPermissionsTable < ActiveRecord::Migration[8.0] + def change + drop_table :permissions + end +end diff --git a/db/schema.rb b/db/schema.rb index 22133e506..81bbe61b2 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[8.0].define(version: 2025_01_15_111655) do +ActiveRecord::Schema[8.0].define(version: 2025_01_18_025829) do create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -277,16 +277,6 @@ t.index ["user_id", "query_doc_pair_id"], name: "index_judgements_on_user_id_and_query_doc_pair_id", unique: true end - create_table "permissions", id: :integer, charset: "utf8mb3", force: :cascade do |t| - t.integer "user_id" - t.string "model_type", null: false - t.string "action", null: false - t.boolean "on", default: false - t.datetime "created_at", precision: nil, null: false - t.datetime "updated_at", precision: nil, null: false - t.index ["user_id"], name: "index_permissions_user_id" - end - create_table "queries", id: :integer, charset: "utf8mb3", force: :cascade do |t| t.bigint "arranged_next" t.bigint "arranged_at" diff --git a/docs/app_structure.md b/docs/app_structure.md index 949414e1b..312d639d4 100644 --- a/docs/app_structure.md +++ b/docs/app_structure.md @@ -30,25 +30,6 @@ The main entry to the app is through a case page, which is controller by the `ap This is the basic structure of the app and should get you started. -## Permissions Structure - -We currently have a mix of hard coded rules and the use of the Pundit gem. For example, this code from `scorers_controller.rb`: - -``` -unless @scorer.owner == current_user or (@scorer.communal and policy(@scorer).update_communal?) - render( - json: { - error: 'Cannot edit a scorer you do not own', - }, - status: :forbidden - ) - - return -end -``` - -We should be able to just use the `authorize()` and `policy()` methods from Pundit. - ## HTTPS / HTTP Quepid runs on HTTPS where possible, however interacting via JSONP with Solr means that if Solr is under HTTP, then the Quepid page needs to be under HTTP as well. We configure `ssl_options` to ensure that Quepid is under HTTPS for all pages except the main `/` or `CoreController` page, which is HTTP. diff --git a/test/controllers/api/v1/current_user_controller_test.rb b/test/controllers/api/v1/current_user_controller_test.rb index 50e26b0c8..339cb4c0d 100644 --- a/test/controllers/api/v1/current_user_controller_test.rb +++ b/test/controllers/api/v1/current_user_controller_test.rb @@ -56,42 +56,6 @@ class CurrentUserControllerTest < ActionController::TestCase end end end - - describe 'user with edit permissions on scorer' do - describe 'when user is an administrator' do - let(:user) { users(:doug) } - - before do - login_user user - end - - test 'has update communal scorer permissions' do - get :show - assert_response :ok - - body = response.parsed_body - - assert_equal body['permissions']['scorer']['update_communal'], true - end - end - - describe 'when user is NOT an administrator' do - let(:user) { users(:jane) } - - before do - login_user user - end - - test 'doesnt have update communal scorer permissions' do - get :show - assert_response :ok - - body = response.parsed_body - - assert_equal body['permissions']['scorer']['update_communal'], false - end - end - end end end end diff --git a/test/controllers/api/v1/scorers_controller_test.rb b/test/controllers/api/v1/scorers_controller_test.rb index 9b1198b20..88221d663 100644 --- a/test/controllers/api/v1/scorers_controller_test.rb +++ b/test/controllers/api/v1/scorers_controller_test.rb @@ -225,6 +225,7 @@ class ScorersControllerTest < ActionController::TestCase let(:shared_scorer) { scorers(:shared_scorer) } let(:communal_scorer) { scorers(:communal_scorer) } let(:jane) { users(:jane) } + let(:admin) { users(:doug) } test 'return a forbidden error if updating a scorer not owned by user' do put :update, params: { id: shared_scorer.id, scorer: { name: 'new name' } } @@ -248,6 +249,20 @@ class ScorersControllerTest < ActionController::TestCase assert_equal error['error'], 'Cannot edit a scorer you do not own' end + test 'lets a administrator update a communal scorer' do + login_user admin + + put :update, params: { id: communal_scorer.id, scorer: { name: 'new name' } } + + assert_response :ok + + scorer = response.parsed_body + owned_scorer.reload + + assert_equal name, scorer['name'] + assert_equal name, owned_scorer.name + end + test 'respects communal_Scorers_only environment setting' do Rails.application.config.communal_scorers_only = true diff --git a/test/models/permission_test.rb b/test/models/permission_test.rb deleted file mode 100644 index b84be82fe..000000000 --- a/test/models/permission_test.rb +++ /dev/null @@ -1,66 +0,0 @@ -# frozen_string_literal: true - -# == Schema Information -# -# Table name: permissions -# -# id :integer not null, primary key -# action :string(255) not null -# model_type :string(255) not null -# on :boolean default(FALSE) -# created_at :datetime not null -# updated_at :datetime not null -# user_id :integer -# -# Indexes -# -# index_permissions_user_id (user_id) -# - -require 'test_helper' - -class PermissionTest < ActiveSupport::TestCase - describe 'defaults' do - let(:user) { users(:random) } - - test 'sets the on attribute to false by default' do - permission = user.permissions.create(model_type: 'user', action: 'update') - - assert false == permission.on - end - end - - describe 'default user permissions' do - test 'initializes the default set of permissions when a user is created' do - assert_difference 'Permission.count', Permissible::PERMISSIONS.count do - User.create email: 'permission@example.com', password: 'password', agreed: true - end - end - end - - describe 'check user permissions' do - let(:user) { users(:random) } - - test 'returns false if user does not have the permission' do - assert_not user.has_permission? 'foo', 'bar' - end - - test 'returns true if user does have the permission' do - user.permissions.create(model_type: 'user', action: 'update') - - assert user.has_permission? 'user', 'update' - end - - test 'returns false if user is not assigned the permission' do - user.permissions.create(model_type: 'user', action: 'update') - - assert_not user.permitted_to? 'user', 'update' - end - - test 'returns true if user is assigned the permission' do - user.permissions.create(model_type: 'user', action: 'update', on: true) - - assert user.permitted_to? 'user', 'update' - end - end -end diff --git a/test/services/permissions_evaluator_test.rb b/test/services/permissions_evaluator_test.rb deleted file mode 100644 index f29f46af6..000000000 --- a/test/services/permissions_evaluator_test.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -class PermissionsEvaluatorTest < ActiveSupport::TestCase - let(:service) { PermissionsEvaluator.new(user) } - let(:subject) { service.run } - let(:defaults) { Permissible.grouped_permissions } - - describe 'evaluates user permissions' do - describe 'when the user has no set permissions' do - let(:user) { users(:random) } - - describe 'when the policy exists' do - test 'returns the policy value' do - policy = Pundit.policy(user, Case) - - assert_equal subject[:case][:create], policy.create? - end - end - end - - describe 'when the user has defined permissions' do - let(:user) { User.create(email: 'foo@example.com', password: 'bar') } - - before do - user.cases << Case.create(case_name: 'Archived Case') - user.cases.first.mark_archived! - end - - describe 'when the policy exists' do - test 'returns the policy value and does not override it' do - policy = Pundit.policy(user, Case) - - assert_equal subject[:case][:create], policy.create? - end - end - - describe 'when the policy does not exist' do - test 'returns the default permission value' do - permissions = user.permissions_hash - - assert_equal subject[:case][:create_multi], permissions[:case][:create_multi] - end - end - end - end -end From b4f151d6224d182c6608ec4317608ae7c9136667 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Fri, 17 Jan 2025 22:24:09 -0500 Subject: [PATCH 2/4] Update test --- test/controllers/api/v1/scorers_controller_test.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/controllers/api/v1/scorers_controller_test.rb b/test/controllers/api/v1/scorers_controller_test.rb index 88221d663..c0d7cf0af 100644 --- a/test/controllers/api/v1/scorers_controller_test.rb +++ b/test/controllers/api/v1/scorers_controller_test.rb @@ -225,7 +225,7 @@ class ScorersControllerTest < ActionController::TestCase let(:shared_scorer) { scorers(:shared_scorer) } let(:communal_scorer) { scorers(:communal_scorer) } let(:jane) { users(:jane) } - let(:admin) { users(:doug) } + let(:admin) { users(:doug) } test 'return a forbidden error if updating a scorer not owned by user' do put :update, params: { id: shared_scorer.id, scorer: { name: 'new name' } } @@ -251,16 +251,17 @@ class ScorersControllerTest < ActionController::TestCase test 'lets a administrator update a communal scorer' do login_user admin + name = 'Custom Name' - put :update, params: { id: communal_scorer.id, scorer: { name: 'new name' } } + put :update, params: { id: communal_scorer.id, scorer: { name: name } } assert_response :ok scorer = response.parsed_body - owned_scorer.reload + communal_scorer.reload assert_equal name, scorer['name'] - assert_equal name, owned_scorer.name + assert_equal name, communal_scorer.name end test 'respects communal_Scorers_only environment setting' do From be28e7b00d2fd5747bc75a6b9977df00e23295c7 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Fri, 17 Jan 2025 22:55:05 -0500 Subject: [PATCH 3/4] Strip out permissions architecture in front end, and just use admin check directly in the communal scorers logic --- .../components/archive_case/_modal.html | 9 +--- .../archive_case_modal_instance_controller.js | 13 +---- .../archive_search_endpoint/_modal.html | 16 ------ ...arch_endpoint_modal_instance_controller.js | 12 ----- .../case_listing/case_listing_controller.js | 18 +------ .../clone_case/clone_case_controller.js | 49 +++++++------------ .../clone_scorer/clone_scorer_controller.js | 8 --- .../components/delete_case/_modal.html | 3 +- .../delete_case_modal_instance_controller.js | 8 --- .../components/delete_scorer/_modal.html | 3 +- ...delete_scorer_modal_instance_controller.js | 10 ---- .../edit_scorer/edit_scorer_controller.js | 4 +- .../components/judgements/_modal.html | 5 +- .../judgements_modal_instance_controller.js | 9 ---- .../new_scorer/new_scorer_controller.js | 8 --- .../new_team/new_team_controller.js | 8 --- .../components/remove_member/_modal.html | 7 +-- ...remove_member_modal_instance_controller.js | 8 --- .../components/remove_scorer/_modal.html | 8 +-- ...remove_scorer_modal_instance_controller.js | 8 --- .../components/share_case/_modal.html | 11 ++--- .../share_case_modal_instance_controller.js | 10 ---- .../components/share_scorer/_modal.html | 12 ++--- .../share_scorer_modal_instance_controller.js | 10 ---- app/assets/javascripts/controllers/case.js | 30 ++++-------- app/assets/javascripts/services/userSvc.js | 2 +- app/views/api/v1/users/_user.json.jbuilder | 1 + .../api/v1/current_user_controller_test.rb | 1 + 28 files changed, 53 insertions(+), 238 deletions(-) diff --git a/app/assets/javascripts/components/archive_case/_modal.html b/app/assets/javascripts/components/archive_case/_modal.html index 0457d65aa..a9b344a39 100644 --- a/app/assets/javascripts/components/archive_case/_modal.html +++ b/app/assets/javascripts/components/archive_case/_modal.html @@ -3,14 +3,9 @@

Archive This Case for Later?

-

You're about to put this case into deep freeze. You'll be able to unarchive it later through the "Archived Cases" filter in the Case Listings.

-

You do not have delete (and archive) permissions for cases.

-

-

+

You're about to put this case into deep freeze. You'll be able to unarchive it later through the "Archived Cases" filter in the Case Listings.

- +
diff --git a/app/assets/javascripts/components/archive_case/archive_case_modal_instance_controller.js b/app/assets/javascripts/components/archive_case/archive_case_modal_instance_controller.js index 1ec9fddf6..629c1cef3 100644 --- a/app/assets/javascripts/components/archive_case/archive_case_modal_instance_controller.js +++ b/app/assets/javascripts/components/archive_case/archive_case_modal_instance_controller.js @@ -8,18 +8,7 @@ angular.module('QuepidApp') function ($rootScope, $uibModalInstance, theCase) { var ctrl = this; - ctrl.theCase = theCase; - ctrl.canDelete = false; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canDelete = $rootScope.currentUser.permissions.case.delete; - } - }); - - ctrl.isOwnerOfCase = function() { - return ($rootScope.currentUser.id === ctrl.theCase.ownerId); - }; + ctrl.theCase = theCase; ctrl.ok = function () { $uibModalInstance.close(true); diff --git a/app/assets/javascripts/components/archive_search_endpoint/_modal.html b/app/assets/javascripts/components/archive_search_endpoint/_modal.html index a0666b10e..e69de29bb 100644 --- a/app/assets/javascripts/components/archive_search_endpoint/_modal.html +++ b/app/assets/javascripts/components/archive_search_endpoint/_modal.html @@ -1,16 +0,0 @@ -
- -

Archive This Search Endpoint for Later?

-
-
-

You're about to put this search endpoint into deep freeze. You'll be able to unarchive it later through the "Archived Search Endpoints" filter in the Search Endpoint Listings.

-

You do not have delete (and archive) permissions for search endpoints.

-

-

-
-
- - -
diff --git a/app/assets/javascripts/components/archive_search_endpoint/archive_search_endpoint_modal_instance_controller.js b/app/assets/javascripts/components/archive_search_endpoint/archive_search_endpoint_modal_instance_controller.js index a4d17f4d3..5b77dd77f 100644 --- a/app/assets/javascripts/components/archive_search_endpoint/archive_search_endpoint_modal_instance_controller.js +++ b/app/assets/javascripts/components/archive_search_endpoint/archive_search_endpoint_modal_instance_controller.js @@ -9,18 +9,6 @@ angular.module('QuepidApp') var ctrl = this; ctrl.theSearchEndpoint = theSearchEndpoint; - //ctrl.canDelete = false; - ctrl.canDelete = true; // hard code that anyone can delete ;-( - - //$rootScope.$watch('currentUser', function() { - // if ( $rootScope.currentUser ) { - // ctrl.canDelete = $rootScope.currentUser.permissions.search_endpoint.delete; - // } - //}); - - ctrl.isOwnerOfSearchEndpoint = function() { - return ($rootScope.currentUser.id === ctrl.theSearchEndpoint.ownerId); - }; ctrl.ok = function () { $uibModalInstance.close(true); diff --git a/app/assets/javascripts/components/case_listing/case_listing_controller.js b/app/assets/javascripts/components/case_listing/case_listing_controller.js index 9e414949d..5e397ec78 100644 --- a/app/assets/javascripts/components/case_listing/case_listing_controller.js +++ b/app/assets/javascripts/components/case_listing/case_listing_controller.js @@ -7,14 +7,12 @@ angular.module('QuepidApp') '$rootScope', '$scope', '$location', - 'flash', 'caseTryNavSvc', 'caseSvc', function ( $rootScope, $scope, $location, - flash, caseTryNavSvc, caseSvc ) { @@ -25,7 +23,6 @@ angular.module('QuepidApp') ctrl.clickToEdit.oldVal = ctrl.thisCase.caseName.slice(0); ctrl.clickToEdit.currVal = ctrl.thisCase.caseName.slice(0); ctrl.clickToEdit.clicked = false; - ctrl.canUpdate = false; // Functions ctrl.cancel = cancel; @@ -39,19 +36,8 @@ angular.module('QuepidApp') $location.path(path); } - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canUpdate = $rootScope.currentUser.permissions.case.update; - } - }); - - function rename() { - if (ctrl.canUpdate) { - ctrl.clickToEdit.clicked = true; - } - else { - flash.error = 'You do not have update permissions for cases.'; - } + function rename() { + ctrl.clickToEdit.clicked = true; } function cancel() { diff --git a/app/assets/javascripts/components/clone_case/clone_case_controller.js b/app/assets/javascripts/components/clone_case/clone_case_controller.js index 032df3918..cc8a2fc55 100644 --- a/app/assets/javascripts/components/clone_case/clone_case_controller.js +++ b/app/assets/javascripts/components/clone_case/clone_case_controller.js @@ -37,39 +37,26 @@ angular.module('QuepidApp') }); } - function prompt() { - if ( !$rootScope.currentUser.permissions.case.create ) { - var deniedModalInstance = $uibModal.open({ - templateUrl: 'new_case/_denied_modal.html', - controller: 'DeniedNewCaseModalInstanceCtrl', - controllerAs: 'ctrl' - }); - - deniedModalInstance.result.then( - function() { }, - function() { } - ); - } else { - var modalInstance = $uibModal.open({ - templateUrl: 'clone_case/_modal.html', - controller: 'CloneCaseModalInstanceCtrl', - controllerAs: 'ctrl', - resolve: { - theCase: function() { - return ctrl.acase; - } + function prompt() { + var modalInstance = $uibModal.open({ + templateUrl: 'clone_case/_modal.html', + controller: 'CloneCaseModalInstanceCtrl', + controllerAs: 'ctrl', + resolve: { + theCase: function() { + return ctrl.acase; } - }); + } + }); - modalInstance.result.then( - function (options) { - ctrl.cloneCase(options); - }, - function() { - $log.info('INFO: Modal dismissed'); - } - ); - } + modalInstance.result.then( + function (options) { + ctrl.cloneCase(options); + }, + function() { + $log.info('INFO: Modal dismissed'); + } + ); } } ]); diff --git a/app/assets/javascripts/components/clone_scorer/clone_scorer_controller.js b/app/assets/javascripts/components/clone_scorer/clone_scorer_controller.js index 0b16c38c0..e7a3edb2b 100644 --- a/app/assets/javascripts/components/clone_scorer/clone_scorer_controller.js +++ b/app/assets/javascripts/components/clone_scorer/clone_scorer_controller.js @@ -21,14 +21,6 @@ angular.module('QuepidApp') var ctrl = this; ctrl.buttonText = $scope.buttonText; - ctrl.cannotCreate = true; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.cannotCreate = !$rootScope.currentUser.permissions.team.create; - } - }); - // Functions ctrl.cloneScorer = cloneScorer; diff --git a/app/assets/javascripts/components/delete_case/_modal.html b/app/assets/javascripts/components/delete_case/_modal.html index e7ff76ead..7709ef28e 100644 --- a/app/assets/javascripts/components/delete_case/_modal.html +++ b/app/assets/javascripts/components/delete_case/_modal.html @@ -3,8 +3,7 @@

Delete This Case

-

You're about to delete this case forever! If you think you might want it later, use the Archive function instead.

-

You do not have delete permissions for cases.

+

You're about to delete this case forever! If you think you might want it later, use the Archive function instead.

diff --git a/app/assets/javascripts/components/delete_case/delete_case_modal_instance_controller.js b/app/assets/javascripts/components/delete_case/delete_case_modal_instance_controller.js index 748dd8d23..22ae973b5 100644 --- a/app/assets/javascripts/components/delete_case/delete_case_modal_instance_controller.js +++ b/app/assets/javascripts/components/delete_case/delete_case_modal_instance_controller.js @@ -7,14 +7,6 @@ angular.module('QuepidApp') function ($rootScope, $uibModalInstance) { var ctrl = this; - ctrl.canDelete = false; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canDelete = $rootScope.currentUser.permissions.case.delete; - } - }); - ctrl.ok = function () { $uibModalInstance.close(true); }; diff --git a/app/assets/javascripts/components/delete_scorer/_modal.html b/app/assets/javascripts/components/delete_scorer/_modal.html index 88538b06e..7a5508480 100644 --- a/app/assets/javascripts/components/delete_scorer/_modal.html +++ b/app/assets/javascripts/components/delete_scorer/_modal.html @@ -3,8 +3,7 @@

Delete This Scorer

-

You're about to delete this scorer forever!

-

You do not have delete permissions for scorers.

+

You're about to delete this scorer forever!

diff --git a/app/assets/javascripts/components/delete_scorer/delete_scorer_modal_instance_controller.js b/app/assets/javascripts/components/delete_scorer/delete_scorer_modal_instance_controller.js index 384779b1e..ecdbb5b48 100644 --- a/app/assets/javascripts/components/delete_scorer/delete_scorer_modal_instance_controller.js +++ b/app/assets/javascripts/components/delete_scorer/delete_scorer_modal_instance_controller.js @@ -7,16 +7,6 @@ angular.module('QuepidApp') function ($rootScope, $uibModalInstance) { var ctrl = this; - // the whole canDelete this may not make sense to have as we don't really support - // changing up these permissions.. - ctrl.canDelete = false; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canDelete = $rootScope.currentUser.permissions.scorer.delete; - } - }); - ctrl.ok = function () { $uibModalInstance.close(true); }; diff --git a/app/assets/javascripts/components/edit_scorer/edit_scorer_controller.js b/app/assets/javascripts/components/edit_scorer/edit_scorer_controller.js index 579f4c38e..7c83e46ab 100644 --- a/app/assets/javascripts/components/edit_scorer/edit_scorer_controller.js +++ b/app/assets/javascripts/components/edit_scorer/edit_scorer_controller.js @@ -26,7 +26,7 @@ angular.module('QuepidApp') function editScorer() { $log.info('INFO: Opened modal to edit scorer!'); - if ( ctrl.lastSaved.communal && !$rootScope.currentUser.permissions.scorer.update_communal ) { + if ( ctrl.lastSaved.communal && !$rootScope.currentUser.administrator ) { var deniedModalInstance = $uibModal.open({ templateUrl: 'edit_scorer/_denied_communal_modal.html', controller: 'DeniedEditScorerModalInstanceCtrl', @@ -38,7 +38,7 @@ angular.module('QuepidApp') function() { } ); } - else if ( !ctrl.lastSaved.communal && !$rootScope.currentUser.permissions.scorer.update ) { + else if ( !ctrl.lastSaved.communal && !$rootScope.currentUser.administrator ) { var deniedModalInstance2 = $uibModal.open({ templateUrl: 'edit_scorer/_denied_modal.html', controller: 'DeniedEditScorerModalInstanceCtrl', diff --git a/app/assets/javascripts/components/judgements/_modal.html b/app/assets/javascripts/components/judgements/_modal.html index c81de8d34..dc1baa47a 100644 --- a/app/assets/javascripts/components/judgements/_modal.html +++ b/app/assets/javascripts/components/judgements/_modal.html @@ -2,11 +2,8 @@

Judgements

-
-

You do not have edit permissions for cases.

-
-
+

Loading your teams, this will be quick I promise! diff --git a/app/assets/javascripts/components/judgements/judgements_modal_instance_controller.js b/app/assets/javascripts/components/judgements/judgements_modal_instance_controller.js index 3e1b23902..151b418eb 100644 --- a/app/assets/javascripts/components/judgements/judgements_modal_instance_controller.js +++ b/app/assets/javascripts/components/judgements/judgements_modal_instance_controller.js @@ -26,20 +26,11 @@ angular.module('QuepidApp') ) { var ctrl = this; - ctrl.canUpdateCase = false; - ctrl.canCreateTeam = false; ctrl.refreshOnly = false; ctrl.updateAssociatedBook = false; ctrl.populateJudgements = false; ctrl.createMissingQueries = false; - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canUpdateCase = $rootScope.currentUser.permissions.case.update; - ctrl.canCreateTeam = $rootScope.currentUser.permissions.team.create; - } - }); - // why do we do this pattern? ctrl.share = { acase: acase, diff --git a/app/assets/javascripts/components/new_scorer/new_scorer_controller.js b/app/assets/javascripts/components/new_scorer/new_scorer_controller.js index e2001da26..c32a02964 100644 --- a/app/assets/javascripts/components/new_scorer/new_scorer_controller.js +++ b/app/assets/javascripts/components/new_scorer/new_scorer_controller.js @@ -23,14 +23,6 @@ angular.module('QuepidApp') var ctrl = this; ctrl.buttonText = $scope.buttonText; - ctrl.cannotCreate = true; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.cannotCreate = !$rootScope.currentUser.permissions.team.create; - } - }); - // Functions ctrl.newScorer = newScorer; diff --git a/app/assets/javascripts/components/new_team/new_team_controller.js b/app/assets/javascripts/components/new_team/new_team_controller.js index 47904dc8f..ad7fad4c2 100644 --- a/app/assets/javascripts/components/new_team/new_team_controller.js +++ b/app/assets/javascripts/components/new_team/new_team_controller.js @@ -21,14 +21,6 @@ angular.module('QuepidApp') var ctrl = this; ctrl.buttonText = $scope.buttonText; - ctrl.cannotCreate = true; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.cannotCreate = !$rootScope.currentUser.permissions.team.create; - } - }); - // Functions ctrl.newTeam = newTeam; diff --git a/app/assets/javascripts/components/remove_member/_modal.html b/app/assets/javascripts/components/remove_member/_modal.html index 50d8e89a7..8f587b993 100644 --- a/app/assets/javascripts/components/remove_member/_modal.html +++ b/app/assets/javascripts/components/remove_member/_modal.html @@ -3,14 +3,11 @@

Remove this Team Member?

-
-

You do not have edit permissions for teams.

-
-
+

You're about to remove {{ ctrl.thisMember.display_name }} from the team {{ ctrl.thisTeam.name }}.

- +
diff --git a/app/assets/javascripts/components/remove_member/remove_member_modal_instance_controller.js b/app/assets/javascripts/components/remove_member/remove_member_modal_instance_controller.js index 09a2f0aad..b630328fd 100644 --- a/app/assets/javascripts/components/remove_member/remove_member_modal_instance_controller.js +++ b/app/assets/javascripts/components/remove_member/remove_member_modal_instance_controller.js @@ -14,14 +14,6 @@ angular.module('QuepidApp') ) { var ctrl = this; - ctrl.canUpdateTeam = false; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canUpdateTeam = $rootScope.currentUser.permissions.team.update; - } - }); - this.thisMember = thisMember; this.thisTeam = thisTeam; diff --git a/app/assets/javascripts/components/remove_scorer/_modal.html b/app/assets/javascripts/components/remove_scorer/_modal.html index f39eb3212..4c90cd8a3 100644 --- a/app/assets/javascripts/components/remove_scorer/_modal.html +++ b/app/assets/javascripts/components/remove_scorer/_modal.html @@ -3,15 +3,11 @@

Remove this Scorer?

-
-

You do not have edit permissions for teams.

-
- -
+

You're about to remove {{ ctrl.thisScorer.name }} from the team {{ ctrl.thisTeam.name }}. This does NOT delete the scorer.

- +
diff --git a/app/assets/javascripts/components/remove_scorer/remove_scorer_modal_instance_controller.js b/app/assets/javascripts/components/remove_scorer/remove_scorer_modal_instance_controller.js index f586298f1..e0f8d18b4 100644 --- a/app/assets/javascripts/components/remove_scorer/remove_scorer_modal_instance_controller.js +++ b/app/assets/javascripts/components/remove_scorer/remove_scorer_modal_instance_controller.js @@ -14,14 +14,6 @@ angular.module('QuepidApp') ) { var ctrl = this; - ctrl.canUpdateTeam = false; - - $rootScope.$watch('currentUser', function() { - if ( $rootScope.currentUser ) { - ctrl.canUpdateTeam = $rootScope.currentUser.permissions.team.update; - } - }); - this.thisScorer = thisScorer; this.thisTeam = thisTeam; diff --git a/app/assets/javascripts/components/share_case/_modal.html b/app/assets/javascripts/components/share_case/_modal.html index 6968b4aa7..b02645ba0 100644 --- a/app/assets/javascripts/components/share_case/_modal.html +++ b/app/assets/javascripts/components/share_case/_modal.html @@ -2,11 +2,8 @@

Share Case

-
-

You do not have edit permissions for cases.

-
-
+

Loading your teams, this will be quick I promise! @@ -47,21 +44,21 @@

Already shared with:

- + Create a team

Share Scorer

-
-

You do not have edit permissions for cases.

-
-
+ +

Loading your teams, this will be quick I promise! @@ -46,21 +44,21 @@

Already shared with:

- + Create a team -

Permission Denied!

-
- -
-

- You don't have permission to create a case. -

- -

- However, since everyone currently has unlimited permissions, this is probably a bug. -

- -

- Please contact Quepid Qommunity and report this! -

-
- -
- - -
diff --git a/app/assets/javascripts/components/new_case/denied_new_case_modal_instance_controller.js b/app/assets/javascripts/components/new_case/denied_new_case_modal_instance_controller.js deleted file mode 100644 index 34c76f3de..000000000 --- a/app/assets/javascripts/components/new_case/denied_new_case_modal_instance_controller.js +++ /dev/null @@ -1,21 +0,0 @@ -'use strict'; - -angular.module('QuepidApp') - .controller('DeniedNewCaseModalInstanceCtrl', [ - '$window', - '$uibModalInstance', - function ( - $window, - $uibModalInstance - ) { - var ctrl = this; - - ctrl.ok = function () { - $window.location.href = '/'; - }; - - ctrl.cancel = function () { - $uibModalInstance.dismiss('cancel'); - }; - } - ]);