From dec23543ddf84354c43d0edf69b92a6a3b7724d6 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 7 Aug 2023 12:06:19 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=81=20Add=20AuthN/AuthZ=20for=20CDL=20?= =?UTF-8?q?assets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, we had not wired in the logic for authorization to controlled digital lending (CDL) assets. With this change, we're adding the logic for handling a user authenticating to access a work. The logic flow is as follows: An unauthenticated user goes to a CDL asset page. They are neither authorized nor authenticated, so we direct them to the new session route. There we capture we're the URL of the requested resource and send them on their way to authenticate. When we perform the handshake with OpenID, we provide (as scope and client design decision) the URL of the CDL asset. The OpenID provider authenticates and returns to the OmniAuth callback. We use the returned scope to then grant authorization to the CDL asset (and revoke any expired authorizations). Related to: - https://github.com/scientist-softserv/palni-palci/issues/633 - https://github.com/scientist-softserv/palni-palci/pull/647 --- app/controllers/devise_decorator.rb | 18 +++++ app/controllers/single_signon_controller.rb | 3 + .../users/omniauth_callbacks_controller.rb | 3 + app/models/work_authorization.rb | 77 ++++++++++++++++++- app/views/hyrax/base/unauthorized.html.erb | 8 ++ .../strategies/open_id_connect_decorator.rb | 56 ++++++++++++++ spec/models/work_authorization_spec.rb | 30 ++++++++ .../open_id_connect_decorator_spec.rb | 65 ++++++++++++++++ 8 files changed, 257 insertions(+), 3 deletions(-) create mode 100644 app/controllers/devise_decorator.rb create mode 100644 app/views/hyrax/base/unauthorized.html.erb create mode 100644 lib/omni_auth/strategies/open_id_connect_decorator.rb create mode 100644 spec/omni_auth/strategies/open_id_connect_decorator_spec.rb diff --git a/app/controllers/devise_decorator.rb b/app/controllers/devise_decorator.rb new file mode 100644 index 000000000..20cd66a12 --- /dev/null +++ b/app/controllers/devise_decorator.rb @@ -0,0 +1,18 @@ +## +# OVERRIDE +# This decorator splices into Devise to set the store URL for potential WorkAuthorization. +module DeviseSessionDecorator + ## + # OVERRIDE + # + # @see OmniAuth::Strategies::OpenIDConnectDecorator#requested_work_url + # @see # https://github.com/scientist-softserv/palni-palci/issues/633 + def new + prepare_for_conditional_work_authorization! + + super + end +end + +Devise::SessionsController.prepend(WorkAuthorization::StoreUrlForScope) +Devise::SessionsController.prepend(DeviseSessionDecorator) diff --git a/app/controllers/single_signon_controller.rb b/app/controllers/single_signon_controller.rb index a17818a56..14fe8322c 100644 --- a/app/controllers/single_signon_controller.rb +++ b/app/controllers/single_signon_controller.rb @@ -1,7 +1,10 @@ # frozen_string_literal: true class SingleSignonController < DeviseController + include WorkAuthorization::StoreUrlForScope + def index + prepare_for_conditional_work_authorization! @identity_providers = IdentityProvider.all render && return unless @identity_providers.count.zero? redirect_to main_app.new_user_session_path diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index acf6868ef..f7bba8e4e 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -10,6 +10,9 @@ def callback @user = User.from_omniauth(request.env['omniauth.auth']) if @user.persisted? + # https://github.com/scientist-softserv/palni-palci/issues/633 + WorkAuthorization.handle_signin_for!(user: @user, scope: params[:scope]) + # We need to render a loading page here just to set the sesion properly # otherwise the logged in session gets lost during the redirect if params[:action] == 'saml' diff --git a/app/models/work_authorization.rb b/app/models/work_authorization.rb index 520adc4a3..8043fcf04 100644 --- a/app/models/work_authorization.rb +++ b/app/models/work_authorization.rb @@ -24,13 +24,25 @@ def initialize(user:, work_pid:) validates :work_pid, presence: true ## + # When a :user signs in, we want to re-authorize works that are part of their latest + # authentication. We want to de-authorize access to any works that are not part of their recent + # authentication. + # # @param user [User] # @param authorize_until [Time] authorize the given work_pid(s) until this point in time. # @param revoke_expirations_before [Time] expire all authorizations that have expires_at less than or equal to this parameter. # @param work_pid [String, Array] - def self.handle_signin_for!(user:, authorize_until: 1.day.from_now, work_pid: nil, revoke_expirations_before: Time.zone.now) + # @param scope [String] the OpenID scope string that might include additional works to authorize. + # + # @see OmniAuth::Strategies::OpenIDConnectDecorator + # @see .extract_pids_from + def self.handle_signin_for!(user:, authorize_until: 1.day.from_now, work_pid: nil, scope: nil, revoke_expirations_before: Time.zone.now) + Rails.logger.info("#{self.class}.#{__method__} granting authorization to work_pid: #{work_pid.inspect} and scope: #{scope.inspect}.") + + pids = Array.wrap(work_pid) + extract_pids_from(scope: scope) + # Maybe we get multiple pids; let's handle that accordingly - Array.wrap(work_pid).each do |pid| + pids.each do |pid| begin authorize!(user: user, work_pid: pid, expires_at: authorize_until) rescue WorkNotFoundError @@ -38,12 +50,34 @@ def self.handle_signin_for!(user:, authorize_until: 1.day.from_now, work_pid: ni end end - # We re-authorized the above work_pid, so it should not be in this query. + # We re-authorized the above pids, so it should not be in this query. where("user_id = :user_id AND expires_at <= :expires_at", user_id: user.id, expires_at: revoke_expirations_before).pluck(:work_pid).each do |pid| revoke!(user: user, work_pid: pid) end end + ## + # A regular expression that identifies the :work_pid for Hyrax work. + # + # @see .extract_pids_from + REGEXP_TO_MATCH_PID = %r{/concern/(?[^\/]+)/(?[^\?]+)(?\?.*)?} + + ## + # From the given :scope string extract an array of potential work_ids. + # + # @param scope [String] + # @param with_regexp [Regexp] + # + # @return [Array] work pid(s) that we found in the given :scope. + def self.extract_pids_from(scope:, with_regexp: REGEXP_TO_MATCH_PID) + return [] if scope.blank? + + scope.split(/\s+/).map do |scope_element| + match = with_regexp.match(scope_element) + match[:work_pid] if match + end.compact + end + ## # Grant the given :user permission to read the work associated with the given :work_pid. # @@ -91,4 +125,41 @@ def self.revoke!(user:, work_pid:) end end # rubocop:enable Rails/FindBy + + ## + # This module is a controller mixin that assumes access to a `session` object. + # + # Via {#prepare_for_conditional_work_authorization!} provide the logic for storing a URL to later + # be available in an authorization request. + # + # @example + # class ApplicationController < ActionController::Base + # include WorkAuthorization::StoreUrlForScope + # end + module StoreUrlForScope + CDL_SESSION_KEY = 'cdl.requested_work_url' + + ## + # Responsible for storing in the session the URL that is a candidate for Controlled Digital + # Lending (CDL) authorization. + def prepare_for_conditional_work_authorization! + # Note: Accessing `stored_location_for` clears that location from the session; so we need to + # grab it and then set it again. + url = stored_location_for(:user) + store_location_for(:user, url) + + # If this could be a controlled digital lending (CDL) work, let's not rely on the + # stored_location (e.g. something that is part of devise and the value being subject to change). + # Instead, let's set the OmniAuth::Strategies::OpenIDConnectDecorator::CDL_SESSION_KEY key in + # the session. + if WorkAuthorization::REGEXP_TO_MATCH_PID.match(url) + Rails.logger.info("Called #{self.class}##{__method__} to set session['#{WorkAuthorization::StoreUrlForScope::CDL_SESSION_KEY}'] to #{url.inspect}.") + session[WorkAuthorization::StoreUrlForScope::CDL_SESSION_KEY] = url + else + Rails.logger.info("Called #{self.class}##{__method__} with stored location of #{url.inspect}; which is not matching as a work URL.") + # We don't have work URL, let's obliterate this. + session.delete(WorkAuthorization::StoreUrlForScope::CDL_SESSION_KEY) + end + end + end end diff --git a/app/views/hyrax/base/unauthorized.html.erb b/app/views/hyrax/base/unauthorized.html.erb new file mode 100644 index 000000000..1784a1d44 --- /dev/null +++ b/app/views/hyrax/base/unauthorized.html.erb @@ -0,0 +1,8 @@ +

<%= t('.unauthorized') %>

+<% if respond_to?(:curation_concern) && curation_concern %> +

<%= t('.is_private', type: curation_concern.human_readable_type.downcase ) %>

+

<%= t('.id', id: curation_concern.id) %> +<% else %> +

<%= t('.page_is_private') %>

+

You may need to <%= link_to "re-authorize to access this resource", '#' %>.

+<% end %> diff --git a/lib/omni_auth/strategies/open_id_connect_decorator.rb b/lib/omni_auth/strategies/open_id_connect_decorator.rb new file mode 100644 index 000000000..c63ca5267 --- /dev/null +++ b/lib/omni_auth/strategies/open_id_connect_decorator.rb @@ -0,0 +1,56 @@ +module OmniAuth + module Strategies + ## + # OVERRIDE to provide openid {#scope} based on the current session. + # + # @see https://github.com/scientist-softserv/palni-palci/issues/633 + module OpenIDConnectDecorator + ## + # In OmniAuth, the options are a tenant wide configuration. However, per + # the client's controlled digitial lending (CDL) system, in the options we + # use for authentication we must inclue the URL of the work that the + # authenticating user wants to access. + # + # @note In testing, when the scope did not include the sample noted in the + # {#requested_work_url} method, the openid provider would return the + # status 200 (e.g. Success) and a body "Failed to get response from + # patron API" + # + # @return [Hash] + def options + # If we don't include this, we keep adding to the `options[:scope]` + return @decorated_options if defined? @decorated_options + + # TODO: What if we are not in the reshare tenant but the tenant is using + # OpenID for authentication? + opts = super + + opts[:scope] += [requested_work_url] unless opts[:scope].include?(requested_work_url) + + Rails.logger.info("#{self.class}#options scope value is #{opts[:scope].inspect}") + + @decorated_options = opts + end + + ## + # @return [String] The URL of the work that was requested by the + # authenticating user. + # + # @note {#session} method is `@env['rack.session']` + # @note {#env} method is the hash representation of the Rack environment. + # @note {#request} method is the {#env} as a Rack::Request object + # + # @note The following URL is known to be acceptable for the reshare.commons-archive.org tenant: + # + # https://reshare.palni-palci-staging.notch8.cloud/concern/cdls/74ebfc53-ee7c-4dc9-9dd7-693e4d840745 + def requested_work_url + Rails.logger.info("#{self.class}#session['#{WorkAuthorization::StoreUrlForScope::CDL_SESSION_KEY}'] is #{session[WorkAuthorization::StoreUrlForScope::CDL_SESSION_KEY].inspect}") + Rails.logger.info("#{self.class}#params['scope'] is #{params['scope'].inspect}") + session[WorkAuthorization::StoreUrlForScope::CDL_SESSION_KEY] || + Array.wrap(params['scope']).split(%r{\s+}).reject { |el| el.to_s == "openid" }.first + end + end + end +end + +OmniAuth::Strategies::OpenIDConnect.prepend(OmniAuth::Strategies::OpenIDConnectDecorator) diff --git a/spec/models/work_authorization_spec.rb b/spec/models/work_authorization_spec.rb index cb73996e5..884ea8d69 100644 --- a/spec/models/work_authorization_spec.rb +++ b/spec/models/work_authorization_spec.rb @@ -8,7 +8,37 @@ let(:borrowing_user) { FactoryBot.create(:user) } let(:ability) { ::Ability.new(borrowing_user) } + describe '.extract_pids_from' do + subject { described_class.extract_pids_from(scope: given_test_scope) } + + [ + ["http://pals.hyku.test/concern/generic_works/f2af2a68-7c79-481b-815e-a91517e23761?locale=en openid", ["f2af2a68-7c79-481b-815e-a91517e23761"]], + ["openid", []], + ["http://pals.hyku.test/concern/generic_works/f2af2a68-7c79-481b-815e-a91517e23761?locale=en", ["f2af2a68-7c79-481b-815e-a91517e23761"]], + ["http://pals.hyku.test/collection/f2af2a68-7c79-481b-815e-a91517e23761?locale=en", []], + [nil, []] + ].each do |given_scope, expected_value| + context "with #{given_scope.inspect}" do + let(:given_test_scope) { given_scope } + + it { is_expected.to match_array(expected_value) } + end + end + end + describe '.handle_signin_for!' do + context 'when given a work_pid and a scope' do + it 'will authorize the given work_pid and scope’s work' do + given_scope = "http://pals.hyku.test/concern/generic_works/#{other_work.id}?locale=en openid" + + expect do + expect do + described_class.handle_signin_for!(user: borrowing_user, work_pid: work.id, scope: given_scope, authorize_until: 1.day.from_now) + end.to change { ::Ability.new(borrowing_user).can?(:read, work.id) }.from(false).to(true) + end.to change { ::Ability.new(borrowing_user).can?(:read, other_work.id) }.from(false).to(true) + end + end + context 'when given a work_pid' do it 'will re-authorize the given work_pid and expire non-specified work_ids' do described_class.authorize!(user: borrowing_user, work_pid: work.id, expires_at: 1.day.ago) diff --git a/spec/omni_auth/strategies/open_id_connect_decorator_spec.rb b/spec/omni_auth/strategies/open_id_connect_decorator_spec.rb new file mode 100644 index 000000000..3bd09c25f --- /dev/null +++ b/spec/omni_auth/strategies/open_id_connect_decorator_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# rubocop:disable RSpec/MultipleDescribes +RSpec.describe OmniAuth::Strategies::OpenIDConnectDecorator do + let(:strategy) do + Class.new do + def initialize(options: {}, session: {}) + @options = options + @session = session + end + attr_reader :options, :session + + # Include this after the attr_reader :options to leverage super method + prepend OmniAuth::Strategies::OpenIDConnectDecorator + end + end + + let(:requested_work_url) { "https://hello.world/something-special" } + let(:options) { { scope: [:openid] } } + let(:session) { { "cdl.requested_work_url" => requested_work_url } } + let(:instance) { strategy.new(options: options, session: session) } + + describe '#options' do + subject { instance.options } + + it "has a :scope key that appends the #requested_work_url" do + expect(subject.fetch(:scope)).to match_array([:openid, requested_work_url]) + end + end + + describe '#requested_work_url' do + subject { instance.requested_work_url } + + it "fetches the 'cdl.requested_work_url' session" do + expect(subject).to eq(requested_work_url) + end + + describe "when the 'cdl.requested_work_url' key is missing" do + let(:session) { {} } + + it { is_expected.to be_nil } + end + end +end + +RSpec.describe OmniAuth::Strategies::OpenIDConnect do + describe '#options method' do + subject(:options_method) { described_class.instance_method(:options) } + + context 'source_location' do + subject { options_method.source_location } + + it { is_expected.to match_array([Rails.root.join('lib', 'omni_auth', 'strategies', 'open_id_connect_decorator.rb').to_s, Integer]) } + end + + context 'super_method' do + subject { options_method.super_method } + + it { is_expected.not_to be_nil } + end + end +end +# rubocop:enable RSpec/MultipleDescribes