From 98cc8aad6aa3e4c4ef59cea6169669aa888b64d7 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Fri, 4 Aug 2023 11:50:50 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=81=20Adding=20WorkAuthorization=20mod?= =?UTF-8?q?el=20for=20CDL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this commit, we're adding a mechanism to "lend" folks books. This change accounts for granting a user read rights to a work. It does not introduce the logic/functionality that "authorizes" or "revokes" this loaned resource. With this commit, there are no changed production logic paths. Related to: - https://github.com/scientist-softserv/palni-palci/issues/633 Co-authored-by: Summer Cook --- app/models/work_authorization.rb | 72 +++++++++++++++++++ .../20230804202804_add_work_authorizations.rb | 13 ++++ db/schema.rb | 24 ++++++- spec/models/work_authorization_spec.rb | 59 +++++++++++++++ 4 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 app/models/work_authorization.rb create mode 100644 db/migrate/20230804202804_add_work_authorizations.rb create mode 100644 spec/models/work_authorization_spec.rb diff --git a/app/models/work_authorization.rb b/app/models/work_authorization.rb new file mode 100644 index 000000000..e7cfc087e --- /dev/null +++ b/app/models/work_authorization.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +# WorkAuthorization models users granted access to works. The instigator of the authorizations is +# outside the model. In the case of PALNI/PALCI there will be a Shibboleth/SAML authentication that +# indicates we should create a WorkAuthorization entry. +# +# @note Transactions across data storage layers (e.g. postgres and fedora) are precarious. Fedora +# doesn't have proper transactions and there is not a clear concept of Postgres and Fedora +# sharing a transaction pool. However, we can emulate one by having a postgres transaction that: +# first does all of the postgres and then does one (ideally single) fedora change. It is +# not bullet proof but does hopefully improve the chances of data integrity. +# +# @see https://github.com/scientist-softserv/palni-palci/issues/633 +class WorkAuthorization < ActiveRecord::Base # rubocop:disable ApplicationRecord + class WorkNotFoundError < StandardError + def initialize(user:, work_pid:) + "Unable to authorize #{user.class} #{user.user_key.inspect} for work with ID=#{work_pid} because work does not exist." + end + end + + belongs_to :user + + # This will be a non-ActiveRecord resource + validates :work_pid, presence: true + + ## + # Grant the given :user permission to read the work associated with the given :work_pid. + # + # @param user [User] + # @param work_pid [String] + # + # @raise [WorkAuthorization::WorkNotFoundError] when the given :work_pid is not found. + # + # @see .revoke! + # rubocop:disable Rails/FindBy + def self.authorize!(user:, work_pid:) + work = ActiveFedora::Base.where(id: work_pid).first + raise WorkNotFoundError.new(user: user, work_pid: work_pid) unless work + + transaction do + authorization = find_or_create_by!(user_id: user.id, work_pid: work.id) + authorization.update!(work_title: work.title) + work.set_read_users([user.user_key], [user.user_key]) + work.save! + end + end + # rubocop:enable Rails/FindBy + + ## + # Remove permission for the given :user to read the work associated with the given :work_pid. + # + # @param user [User] + # @param work_pid [String] + # + # @see .authorize! + # rubocop:disable Rails/FindBy + def self.revoke!(user:, work_pid:) + # When we delete the authorizations, we want to ensure that we've tidied up the corresponding + # work's read users. If for some reason the ActiveFedora save fails, this the destruction of + # the authorizations will rollback. Meaning we still have a record of what we've authorized. + transaction do + where(user_id: user.id, work_pid: work_pid).destroy_all + work = ActiveFedora::Base.where(id: work_pid).first + if work + work.set_read_users([], [user.user_key]) + work.save! + end + true + end + end + # rubocop:enable Rails/FindBy +end diff --git a/db/migrate/20230804202804_add_work_authorizations.rb b/db/migrate/20230804202804_add_work_authorizations.rb new file mode 100644 index 000000000..35ae12fb1 --- /dev/null +++ b/db/migrate/20230804202804_add_work_authorizations.rb @@ -0,0 +1,13 @@ +class AddWorkAuthorizations < ActiveRecord::Migration[5.2] + def change + create_table "work_authorizations", force: :cascade do |t| + t.string "work_title" + t.belongs_to "user" + t.datetime "expires_at", index: true + t.string "work_pid", index: true, null: false + t.string "scope" + t.string "error", default: nil + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 1a6536177..b21235088 100755 --- 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: 2023_07_18_202804) do +ActiveRecord::Schema.define(version: 2023_08_04_202804) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -83,6 +83,9 @@ t.datetime "last_succeeded_at" t.string "importerexporter_type", default: "Bulkrax::Importer" t.integer "import_attempts", default: 0 + t.index ["identifier"], name: "index_bulkrax_entries_on_identifier" + t.index ["importerexporter_id", "importerexporter_type"], name: "bulkrax_entries_importerexporter_idx" + t.index ["type"], name: "index_bulkrax_entries_on_type" end create_table "bulkrax_exporter_runs", force: :cascade do |t| @@ -165,7 +168,9 @@ t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "order", default: 0 + t.index ["child_id"], name: "index_bulkrax_pending_relationships_on_child_id" t.index ["importer_run_id"], name: "index_bulkrax_pending_relationships_on_importer_run_id" + t.index ["parent_id"], name: "index_bulkrax_pending_relationships_on_parent_id" end create_table "bulkrax_statuses", force: :cascade do |t| @@ -179,6 +184,9 @@ t.string "runnable_type" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index ["error_class"], name: "index_bulkrax_statuses_on_error_class" + t.index ["runnable_id", "runnable_type"], name: "bulkrax_statuses_runnable_idx" + t.index ["statusable_id", "statusable_type"], name: "bulkrax_statuses_statusable_idx" end create_table "checksum_audit_logs", id: :serial, force: :cascade do |t| @@ -857,6 +865,20 @@ t.datetime "updated_at" end + create_table "work_authorizations", force: :cascade do |t| + t.string "work_title" + t.bigint "user_id" + t.datetime "expires_at" + t.string "work_pid", null: false + t.string "scope" + t.string "error" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["expires_at"], name: "index_work_authorizations_on_expires_at" + t.index ["user_id"], name: "index_work_authorizations_on_user_id" + t.index ["work_pid"], name: "index_work_authorizations_on_work_pid" + end + create_table "work_view_stats", id: :serial, force: :cascade do |t| t.datetime "date" t.integer "work_views" diff --git a/spec/models/work_authorization_spec.rb b/spec/models/work_authorization_spec.rb new file mode 100644 index 000000000..16ea9440b --- /dev/null +++ b/spec/models/work_authorization_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +require 'cancan/matchers' + +RSpec.describe WorkAuthorization, type: :model do + let(:work) { FactoryBot.create(:generic_work) } + let(:borrowing_user) { FactoryBot.create(:user) } + let(:ability) { ::Ability.new(borrowing_user) } + + describe '.authorize!' do + it 'gives the borrowing user the ability to "read" the work' do + # We re-instantiate an ability class because CanCan caches many of the ability checks. By + # both passing the id and reinstantiating, we ensure that we have the most fresh data; that is + # no cached ability "table" nor cached values on the work. + expect { described_class.authorize!(user: borrowing_user, work_pid: work.id) } + .to change { ::Ability.new(borrowing_user).can?(:read, work.id) }.from(false).to(true) + end + + context 'when the work_pid is not found' do + it 'raises a WorkAuthorization::WorkNotFoundError' do + expect { described_class.authorize!(user: borrowing_user, work_pid: "oh-so-404") }.to raise_error(WorkAuthorization::WorkNotFoundError) + end + end + end + + describe '.revoke!' do + it 'revokes an authorized user from being able to "read" the work' do + # Ensuring we're authorized + described_class.authorize!(user: borrowing_user, work_pid: work.id) + + expect { described_class.revoke!(user: borrowing_user, work_pid: work.id) } + .to change { ::Ability.new(borrowing_user).can?(:read, work.id) }.from(true).to(false) + end + + it 'gracefully handles revocation of non-existent works' do + expect(described_class.revoke!(user: borrowing_user, work_pid: "oh-so-404")).to be_truthy + end + + it 'gracefully handles revoking that which was never authorized' do + expect { described_class.revoke!(user: borrowing_user, work_pid: work.id) } + .not_to change { ::Ability.new(borrowing_user).can?(:read, work.id) }.from(false) + end + end + + xdescribe 'adding errors' do + let(:authorization) { WorkAuthorization.new } + + it 'adds the error' do + authorization.update_error 'test error' + expect(authorization.error).to eq('test error') + end + + it 'clears error' do + authorization.update_error 'test error' + authorization.clear_error + expect(authorization.error).to eq(nil) + end + end +end