From cec7530d4433247b2281e0d39718d5942409a26d Mon Sep 17 00:00:00 2001 From: Martin Fenner Date: Thu, 16 Jan 2020 12:45:06 +0100 Subject: [PATCH] use Elasticsearch query for single DOI. #391 --- app/controllers/application_controller.rb | 1 + app/controllers/dois_controller.rb | 33 +++++++------ app/controllers/works_controller.rb | 10 ++-- app/models/ability.rb | 28 +++--------- app/models/concerns/authenticable.rb | 15 ++++++ app/models/doi.rb | 9 +++- spec/concerns/authenticable_spec.rb | 56 +++++++++++++++++++++++ spec/models/ability_spec.rb | 4 +- spec/requests/dois_spec.rb | 28 ++++-------- spec/requests/works_spec.rb | 22 +++++++++ 10 files changed, 140 insertions(+), 66 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8b3cb79f6..99fd3f4c1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -68,6 +68,7 @@ def authenticate_user! return false if credentials.blank? @current_user = User.new(credentials, type: type) + fail CanCan::AuthorizationNotPerformed if @current_user.errors.present? end def current_ability diff --git a/app/controllers/dois_controller.rb b/app/controllers/dois_controller.rb index d11b2560a..8c6ec672f 100644 --- a/app/controllers/dois_controller.rb +++ b/app/controllers/dois_controller.rb @@ -6,13 +6,10 @@ class DoisController < ApplicationController include Crosscitable prepend_before_action :authenticate_user! - before_action :set_doi, only: [:show, :get_url] before_action :set_include, only: [:index, :show, :create, :update] before_action :set_raven_context, only: [:create, :update, :validate] def index - authorize! :read, Doi - sort = case params[:sort] when "name" then { "doi" => { order: 'asc' }} when "-name" then { "doi" => { order: 'desc' }} @@ -234,10 +231,16 @@ def index end def show - authorize! :read, @doi + # only show findable DOIs to anonymous users and role user + # use current_user role to determine permissions to access draft and registered dois + options = filter_doi_by_role(current_user) + response = Doi.find_by_id(params[:id], options) + fail ActiveRecord::RecordNotFound unless response.results.present? respond_to do |format| format.json do + doi = response.results.first + options = {} options[:include] = @include options[:is_collection] = false @@ -247,15 +250,19 @@ def show affiliation: params[:affiliation] } - render json: DoiSerializer.new(@doi, options).serialized_json, status: :ok + render json: DoiSerializer.new(doi, options).serialized_json, status: :ok end - format.citation do + + # use active_record for content negotiation + doi = response.records.first + + format.citation do # fetch formatted citation - render citation: @doi, style: params[:style] || "apa", locale: params[:locale] || "en-US" + render citation: doi, style: params[:style] || "apa", locale: params[:locale] || "en-US" end header = %w(doi url registered state resourceTypeGeneral resourceType title author publisher publicationYear) - format.any(:bibtex, :citeproc, :codemeta, :crosscite, :datacite, :datacite_json, :jats, :ris, :schema_org) { render request.format.to_sym => @doi } - format.csv { render request.format.to_sym => @doi, header: header } + format.any(:bibtex, :citeproc, :codemeta, :crosscite, :datacite, :datacite_json, :jats, :ris, :schema_org) { render request.format.to_sym => doi } + format.csv { render request.format.to_sym => doi, header: header } end end @@ -403,6 +410,9 @@ def random end def get_url + @doi = Doi.where(doi: params[:id]).first + fail ActiveRecord::RecordNotFound unless @doi.present? + authorize! :get_url, @doi if !@doi.is_registered_or_findable? || %w(europ crossref medra jalc kisti op).include?(@doi.provider_id) || %w(Crossref mEDRA).include?(@doi.agency) @@ -457,11 +467,6 @@ def status protected - def set_doi - @doi = Doi.where(doi: params[:id]).first - fail ActiveRecord::RecordNotFound unless @doi.present? - end - def set_include if params[:include].present? @include = params[:include].split(",").map { |i| i.downcase.underscore.to_sym } diff --git a/app/controllers/works_controller.rb b/app/controllers/works_controller.rb index 337573e14..92fce0e21 100644 --- a/app/controllers/works_controller.rb +++ b/app/controllers/works_controller.rb @@ -1,11 +1,8 @@ class WorksController < ApplicationController - prepend_before_action :authenticate_user! before_action :set_doi, only: [:show] before_action :set_include, only: [:index, :show] def index - authorize! :read, Doi - sort = case params[:sort] when "name" then { "doi" => { order: 'asc' }} when "-name" then { "doi" => { order: 'desc' }} @@ -106,8 +103,6 @@ def index end def show - authorize! :read, @doi - options = {} options[:include] = @include options[:is_collection] = false @@ -122,8 +117,9 @@ def show protected def set_doi - response = Doi.find_by_id(params[:id]) - @doi = response.records.first + options = filter_doi_by_role(current_user) + response = Doi.find_by_id(params[:id], options) + @doi = response.results.first fail ActiveRecord::RecordNotFound unless @doi.present? end diff --git a/app/models/ability.rb b/app/models/ability.rb index 8cbd0d458..7822a5278 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -34,9 +34,7 @@ def initialize(user) # end can [:read, :get_url, :transfer, :read_landing_page_results], Doi, :provider_id => user.provider_id - can [:read], Doi do |doi| - doi.findable? - end + can [:read], Doi can [:read], User can [:read], Phrase can [:read], Activity do |activity| @@ -49,9 +47,7 @@ def initialize(user) can [:read], Client, :provider_id => user.provider_id can [:read], ClientPrefix#, :client_id => user.client_id can [:read, :get_url, :read_landing_page_results], Doi, :provider_id => user.provider_id - can [:read], Doi do |doi| - doi.findable? - end + can [:read], Doi can [:read], User can [:read], Phrase can [:read], Activity do |activity| @@ -72,9 +68,7 @@ def initialize(user) can [:new, :create], Doi do |doi| doi.client.prefixes.where(prefix: doi.prefix).present? || %w(crossref medra kisti jalc op).include?(doi.client.symbol.downcase.split(".").first) end - can [:read], Doi do |doi| - doi.findable? - end + can [:read], Doi can [:read], User can [:read], Phrase can [:read], Activity do |activity| @@ -85,9 +79,7 @@ def initialize(user) can [:read], Client, :symbol => user.client_id.upcase can [:read], ClientPrefix, :client_id => user.client_id can [:read, :get_url, :read_landing_page_results], Doi, :client_id => user.client_id - can [:read], Doi do - |doi| doi.findable? - end + can [:read], Doi can [:read], User can [:read], Phrase can [:read], Activity do |activity| @@ -98,9 +90,7 @@ def initialize(user) can [:update], Provider, :symbol => user.provider_id.upcase if user.provider_id.present? can [:read, :update], Client, :symbol => user.client_id.upcase if user.client_id.present? can [:read], Doi, :client_id => user.client_id if user.client_id.present? - can [:read, :get_url], Doi do |doi| - doi.findable? - end + can [:read, :get_url], Doi can [:read], User, :id => user.id can [:read], Phrase can [:read], Activity do |activity| @@ -112,18 +102,14 @@ def initialize(user) can [:update], Provider, :symbol => user.provider_id.upcase if user.provider_id.present? can [:read, :update], Client, :symbol => user.client_id.upcase if user.client_id.present? can [:read], Doi, :client_id => user.client_id if user.client_id.present? - can [:read, :get_url], Doi do |doi| - doi.findable? - end + can [:read, :get_url], Doi can [:read], User, :id => user.id can [:read], Phrase can [:read], Activity do |activity| activity.doi.findable? end elsif user.role_id == "anonymous" - can [:read, :get_url], Doi do |doi| - doi.findable? - end + can [:read, :get_url], Doi can [:read], Provider can [:read], Activity do |activity| activity.doi.findable? diff --git a/app/models/concerns/authenticable.rb b/app/models/concerns/authenticable.rb index 8ce471b22..bcd415c2a 100644 --- a/app/models/concerns/authenticable.rb +++ b/app/models/concerns/authenticable.rb @@ -152,6 +152,21 @@ def secure_compare(a, b) b.each_byte { |byte| res |= byte ^ l.shift } res == 0 end + + # filter results based on user permissions + def filter_doi_by_role(user) + return { state: "findable" } if user.blank? + + if %w(staff_admin staff_user).include?(user.role_id) + {} + elsif %w(provider_admin provider_user).include?(user.role_id) && user.provider_id.present? + { provider_id: user.provider_id } + elsif %w(client_admin client_user user temporary).include?(user.role_id) && user.client_id.present? + { client_id: user.client_id } + else + { state: "findable" } + end + end end module ClassMethods diff --git a/app/models/doi.rb b/app/models/doi.rb index 2dfd15202..da01d745f 100644 --- a/app/models/doi.rb +++ b/app/models/doi.rb @@ -518,13 +518,18 @@ def self.find_by_id(ids, options={}) options[:page][:size] ||= 1000 options[:sort] ||= { created: { order: "asc" }} + must = [{ terms: { doi: ids.map(&:upcase) }}] + must << { terms: { aasm_state: options[:state].to_s.split(",") }} if options[:state].present? + must << { terms: { provider_id: options[:provider_id].split(",") }} if options[:provider_id].present? + must << { terms: { client_id: options[:client_id].to_s.split(",") }} if options[:client_id].present? + __elasticsearch__.search({ from: (options.dig(:page, :number) - 1) * options.dig(:page, :size), size: options.dig(:page, :size), sort: [options[:sort]], query: { - terms: { - doi: ids.map(&:upcase) + bool: { + must: must } }, aggregations: query_aggregations diff --git a/spec/concerns/authenticable_spec.rb b/spec/concerns/authenticable_spec.rb index 734cc4ab3..c76278f65 100644 --- a/spec/concerns/authenticable_spec.rb +++ b/spec/concerns/authenticable_spec.rb @@ -54,6 +54,62 @@ end end + describe "filter_doi_by_role" do + it "staff_admin" do + token = User.generate_token(role_id: "staff_admin") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq({}) + end + + it "staff_user" do + token = User.generate_token(role_id: "staff_user") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq({}) + end + + it "provider_admin" do + token = User.generate_token(role_id: "provider_admin", provider_id: "datacite") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(:provider_id=>"datacite") + end + + it "provider_user" do + token = User.generate_token(role_id: "provider_user", provider_id: "datacite") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(:provider_id=>"datacite") + end + + it "client_admin" do + token = User.generate_token(role_id: "client_admin", client_id: "datacite.rph") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(client_id: "datacite.rph") + end + + it "client_user" do + token = User.generate_token(role_id: "client_user", client_id: "datacite.rph") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(client_id: "datacite.rph") + end + + it "user" do + token = User.generate_token(role_id: "user") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(:state=>"findable") + end + + it "temporary" do + token = User.generate_token(role_id: "temporary") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(:state=>"findable") + end + + it "anonymous" do + token = User.generate_token(role_id: "anonymous") + subject = User.new(token) + expect(subject.filter_doi_by_role(subject)).to eq(:state=>"findable") + end + end + describe 'encode_token' do it "with name" do token = subject.encode_token("name" => "Josiah Carberry") diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index b42823454..79554d8eb 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -43,7 +43,7 @@ it{ is_expected.not_to be_able_to(:update, prefix) } it{ is_expected.not_to be_able_to(:destroy, prefix) } - it{ is_expected.not_to be_able_to(:read, doi) } + it{ is_expected.to be_able_to(:read, doi) } it{ is_expected.not_to be_able_to(:transfer, doi) } it{ is_expected.not_to be_able_to(:create, doi) } it{ is_expected.not_to be_able_to(:update, doi) } @@ -232,7 +232,7 @@ it{ is_expected.not_to be_able_to(:update, client) } it{ is_expected.not_to be_able_to(:destroy, client) } - it{ is_expected.not_to be_able_to(:read, doi) } + it{ is_expected.to be_able_to(:read, doi) } it{ is_expected.not_to be_able_to(:transfer, doi) } it{ is_expected.not_to be_able_to(:create, doi) } it{ is_expected.not_to be_able_to(:update, doi) } diff --git a/spec/requests/dois_spec.rb b/spec/requests/dois_spec.rb index 96cfd9717..bfa01641e 100644 --- a/spec/requests/dois_spec.rb +++ b/spec/requests/dois_spec.rb @@ -81,20 +81,8 @@ it 'returns the Doi' do get "/dois/#{doi.doi}" - expect(last_response.status).to eq(401) - expect(json.fetch('errors')).to eq([{"status"=>"401", "title"=>"Bad credentials."}]) - end - end - - context 'invalid password' do - let(:bearer) { Client.generate_token(role_id: "client_admin", uid: client.symbol, provider_id: provider.symbol.downcase, client_id: client.symbol.downcase, password: "abc") } - let(:headers) { { 'HTTP_ACCEPT'=>'application/vnd.api+json', 'HTTP_AUTHORIZATION' => 'Bearer ' + bearer }} - - it 'returns the Doi' do - get "/dois/#{doi.doi}" - - expect(last_response.status).to eq(401) - expect(json.fetch('errors')).to eq([{"status"=>"401", "title"=>"Bad credentials."}]) + expect(last_response.status).to eq(404) + expect(json).to eq("errors"=>[{"status"=>"404", "title"=>"The resource you are looking for doesn't exist."}]) end end @@ -2830,11 +2818,11 @@ let(:bearer) { User.generate_token(role_id: "client_admin", client_id: client.symbol.downcase) } let(:headers) { { 'HTTP_ACCEPT'=>'application/vnd.api+json', 'HTTP_AUTHORIZATION' => 'Bearer ' + bearer } } - it 'returns with landing page results' do + it 'returns without landing page results' do get "/dois/#{doi.doi}", nil, headers expect(json.dig('data', 'attributes', 'doi')).to eq(doi.doi) - expect(json.dig('data', 'attributes', 'landingPage')).to eq(landing_page) + expect(json.dig('data', 'attributes', 'landingPage')).to be_nil end end @@ -3001,8 +2989,8 @@ it 'returns error message' do get "/dois/#{doi.doi}", nil, { "HTTP_ACCEPT" => "application/vnd.jats+xml", 'HTTP_AUTHORIZATION' => 'Bearer ' + bearer } - expect(last_response.status).to eq(403) - expect(json["errors"]).to eq([{"status"=>"403", "title"=>"You are not authorized to access this resource."}]) + expect(last_response.status).to eq(404) + expect(json).to eq("errors"=>[{"status"=>"404", "title"=>"The resource you are looking for doesn't exist."}]) end end @@ -3012,8 +3000,8 @@ it 'returns error message' do get "/dois/#{doi.doi}", nil, { "HTTP_ACCEPT" => "application/vnd.jats+xml" } - expect(last_response.status).to eq(401) - expect(json["errors"]).to eq([{"status"=>"401", "title"=>"Bad credentials."}]) + expect(last_response.status).to eq(404) + expect(json).to eq("errors"=>[{"status"=>"404", "title"=>"The resource you are looking for doesn't exist."}]) end end diff --git a/spec/requests/works_spec.rb b/spec/requests/works_spec.rb index 680c83c57..db503a7df 100644 --- a/spec/requests/works_spec.rb +++ b/spec/requests/works_spec.rb @@ -61,6 +61,17 @@ end end + context 'draft doi' do + let!(:doi) { create(:doi, client: client) } + + it 'returns 404 status' do + get "/works/#{doi.doi}", nil, headers + + expect(last_response.status).to eq(404) + expect(json).to eq("errors"=>[{"status"=>"404", "title"=>"The resource you are looking for doesn't exist."}]) + end + end + context 'anonymous user' do it 'returns the Doi' do get "/works/#{doi.doi}" @@ -69,5 +80,16 @@ expect(json.dig('data', 'attributes', 'doi')).to eq(doi.doi.downcase) end end + + context 'anonymous user draft doi' do + let!(:doi) { create(:doi, client: client) } + + it 'returns 404 status' do + get "/works/#{doi.doi}" + + expect(last_response.status).to eq(404) + expect(json).to eq("errors"=>[{"status"=>"404", "title"=>"The resource you are looking for doesn't exist."}]) + end + end end end