Merge pull request #122 from hathitrust/HT-3054_auth_policies

HT-3054 users with admin ht_users can't admin approval requests

app/controllers/application_controller.rb

@@ -26,9 +26,8 @@ def authorize!
     raise NotAuthorizedError unless can?(params[:action], params[:controller], current_user)
   end
 
-  def can?(action, resource, user = current_user)
-    res = Checkpoint::Resource::AllOfType.new(resource)
-    Checkpoint::Query::ActionPermitted.new(user, action, res, authority: Services.checkpoint).true?
+  def can?(action, object, user = current_user)
+    policy(object).can?(action, authorizable_object(object), user)
   end
 
   # This could be replaced by a landing page that is accessible
@@ -41,6 +40,17 @@ def default_path
 
   private
 
+  # Turn string or symbol referencing ActiveRecord class into that class.
+  # For ActiveRecord objects and classes this is an identity.
+  # Typically this will take something like "ht_users" and return HTUser.
+  # Use this intermediate representation (instead of just converting to a Checkpoint::Resource)
+  # because policies may need to reason about real model objects.
+  def authorizable_object(object)
+    return object if object.respond_to? :to_resource
+
+    Object.const_get(object.to_s.singularize.camelize)
+  end
+
   def user_not_authorized
     # logout
     render_forbidden
@@ -71,6 +81,24 @@ def set_csrf_cookie
     cookies["CSRF-TOKEN"] = form_authenticity_token
   end
 
+  # Derive the policy class from either a controller name like ht_users,
+  # or from an object, typically when more fine-grained access control is
+  # to be enforced.
+  def policy(resource)
+    basename = if [String, Symbol].include?(resource.class)
+      resource.to_s
+    else
+      resource.class.to_s
+    end
+    basename = basename.pluralize.camelize + "Policy"
+    begin
+      policy_class = Object.const_get(basename)
+    rescue NameError => _e
+      policy_class = Object.const_get("ApplicationPolicy")
+    end
+    policy_class.new
+  end
+
   def attributes_factory
     @attributes_factory ||= Keycard::Request::AttributesFactory.new(finders: [])
   end

app/lib/otis/authorization/resource.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Otis
+  module Authorization
+    module Resource
+      def self.included(klass)
+        klass.extend ClassMethods
+      end
+
+      module ClassMethods
+        def to_resource
+          Checkpoint::Resource::AllOfType.new to_s.underscore.to_sym
+        end
+
+        def to_resource_name
+          to_s.underscore.to_sym
+        end
+      end
+
+      def to_resource
+        Checkpoint::Resource.new self
+      end
+
+      def resource_type
+        self.class.to_resource_name
+      end
+
+      def resource_id
+        id
+      end
+    end
+  end
+end

app/models/application_record.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
 class ApplicationRecord < ActiveRecord::Base
+  include Otis::Authorization::Resource
+
   self.abstract_class = true
 end

app/models/ht_approval_request.rb

@@ -44,15 +44,6 @@ def self.find_by_token(tok)
     find_by_token_hash(digest(tok))
   end
 
-  # Checkpoint
-  def resource_type
-    :ht_approval_request
-  end
-
-  def resource_id
-    id
-  end
-
   # This is the bit that goes to the approver, just a gob of b64 data acting as a 'password'
   def token
     @token ||= SecureRandom.urlsafe_base64 16

app/models/ht_contact.rb

@@ -14,15 +14,6 @@ class HTContact < ApplicationRecord
 
   self.primary_key = "id"
 
-  # Checkpoint
-  def resource_type
-    :ht_contact
-  end
-
-  def resource_id
-    id
-  end
-
   def institution
     HTInstitution.find(self[:inst_id])
   end

app/models/ht_contact_type.rb

@@ -11,15 +11,6 @@ class HTContactType < ApplicationRecord
 
   self.primary_key = "id"
 
-  # Checkpoint
-  def resource_type
-    :ht_contact_type
-  end
-
-  def resource_id
-    id
-  end
-
   private
 
   def check_contacts

app/models/ht_institution.rb

@@ -22,15 +22,6 @@ class HTInstitution < ApplicationRecord
   scope :enabled, -> { where("enabled = 1") }
   scope :other, -> { where("enabled != 1") }
 
-  # Checkpoint
-  def resource_type
-    :ht_institution
-  end
-
-  def resource_id
-    id
-  end
-
   def set_defaults
     self.mapto_inst_id ||= inst_id
     return unless entityID

app/models/ht_user.rb

@@ -41,11 +41,7 @@ def self.human_attribute_name(attr, options = {})
     HUMANIZED_ATTRIBUTES[attr.to_sym] || super
   end
 
-  # Checkpoint
-  def resource_type
-    :ht_user
-  end
-
+  # Checkpoint override
   def resource_id
     email
   end

app/policies/application_policy.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class ApplicationPolicy
+  def can?(action, object, user)
+    return false if object.nil?
+
+    Checkpoint::Query::ActionPermitted.new(user, action,
+      object.to_resource, authority: authority).true?
+  end
+
+  def authority
+    Services.checkpoint
+  end
+end

app/policies/ht_approval_requests_policy.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class HTApprovalRequestsPolicy < ApplicationPolicy
+  def can?(action, object, user)
+    super(action, object, user) || super(action, object.try(:ht_user), user)
+  end
+end

app/views/ht_users/index.html.erb

@@ -10,7 +10,7 @@
          data-search="true" data-show-search-clear-button="true">
     <thead class="thead-dark">
     <tr>
-      <% if can?(:create, :ht_renewal_requests) || can?(:edit, :ht_users) %>
+      <% if can?(:create, :ht_approval_requests) || can?(:edit, :ht_users) %>
         <th data-sortable="true">Select</th>
       <% end %>
       <th data-sortable="true">E-mail</th>
@@ -26,7 +26,7 @@
 
     <% @users.each do |u| %>
       <tr>
-        <% if can?(:create, :ht_renewal_requests) || can?(:edit, :ht_users) %>
+        <% if can?(:create, :ht_approval_requests) || can?(:edit, :ht_users) %>
           <td><%= u.select_for_renewal_checkbox %></td>
         <% end %>
         <td><%= u.email_link %></td>
@@ -50,7 +50,7 @@
   </table>
   <br/>
 
-  <% if can?(:create, :ht_renewal_requests)  %>
+  <% if can?(:create, :ht_approval_requests)  %>
     <%= button_tag 'Create Approval Requests', type: 'submit', name: 'submit_requests', class: 'btn btn-primary' %>
   <% end %>
   <% if can?(:edit, :ht_users) %>

lib/tasks/migrate_users.rake

@@ -16,8 +16,8 @@ namespace :otis do
     admin_role = Checkpoint::Credential::Role.new(:admin)
     view_role = Checkpoint::Credential::Role.new(:view)
     res_wildcard = Checkpoint::Resource::AllOfAnyType.new
-    res_contact = Checkpoint::Resource::AllOfType.new(:ht_contacts)
-    res_contact_type = Checkpoint::Resource::AllOfType.new(:ht_contact_types)
+    res_contact = Checkpoint::Resource::AllOfType.new(:ht_contact)
+    res_contact_type = Checkpoint::Resource::AllOfType.new(:ht_contact_type)
     if Otis.config.users.present?
       Otis.config.users.each do |u|
         agent = Checkpoint::Agent::Token.new("user", u)
@@ -35,7 +35,7 @@ namespace :otis do
       end
     end
     if Otis.config.institution.present?
-      res_inst = Checkpoint::Resource::AllOfType.new(:ht_institutions)
+      res_inst = Checkpoint::Resource::AllOfType.new(:ht_institution)
       Otis.config.institution.each do |u|
         agent = Checkpoint::Agent::Token.new("user", u)
         unless Services.checkpoint.permits?(agent, view_role, res_inst)

test/policies/application_policy_test.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class ApplicationPolicyTest < ActiveSupport::TestCase
+  def setup
+    @user = User.new("[email protected]")
+    @ht_user = create(:ht_user)
+    agent = Checkpoint::Agent::Token.new("user", @user.id)
+    view_role = Checkpoint::Credential::Role.new(:view)
+    res_users = Checkpoint::Resource::AllOfType.new(:ht_user)
+    Services.checkpoint.grant!(agent, view_role, res_users)
+  end
+
+  test "view credential permits ht_users index" do
+    assert ApplicationPolicy.new.can?(:index, HTUser, @user)
+  end
+
+  test "view credential permits showing a particular HTUser" do
+    assert ApplicationPolicy.new.can?(:show, @ht_user, @user)
+  end
+
+  test "view credential forbids deleting a particular HTUser" do
+    refute ApplicationPolicy.new.can?(:delete, @ht_user, @user)
+  end
+end

test/policies/ht_approval_requests_policy_test.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class HTApprovalRequestsPolicyTest < ActiveSupport::TestCase
+  def setup
+    @user = User.new("[email protected]")
+    @editable_req = create(:ht_approval_request)
+    @uneditable_req = create(:ht_approval_request)
+    agent = Checkpoint::Agent::Token.new("user", @user.id)
+    admin_role = Checkpoint::Credential::Role.new(:admin)
+    res_user = Checkpoint::Resource.new(@editable_req.ht_user)
+    Services.checkpoint.grant!(agent, admin_role, res_user)
+  end
+
+  test "admin credential for HTUser permits editing its HTApprovalRequest" do
+    assert HTApprovalRequestsPolicy.new.can?(:edit, @editable_req, @user)
+  end
+
+  test "admin credential for HTUser does not permit editing unrelated HTApprovalRequest" do
+    refute HTApprovalRequestsPolicy.new.can?(:edit, @uneditable_req, @user)
+  end
+end