From 7dd559111b026fcf8909269d63f6e181eb071fa2 Mon Sep 17 00:00:00 2001 From: Martin Fenner Date: Sat, 18 Jan 2020 07:28:03 +0100 Subject: [PATCH] don't use query for doi permission check. #386 --- app/controllers/dois_controller.rb | 14 +-- app/controllers/works_controller.rb | 2 +- app/models/concerns/authenticable.rb | 22 ++-- spec/concerns/authenticable_spec.rb | 170 ++++++++++++++++++--------- spec/requests/dois_spec.rb | 13 ++ 5 files changed, 143 insertions(+), 78 deletions(-) diff --git a/app/controllers/dois_controller.rb b/app/controllers/dois_controller.rb index 14c9872cc..59c934a3c 100644 --- a/app/controllers/dois_controller.rb +++ b/app/controllers/dois_controller.rb @@ -233,17 +233,9 @@ def index def show # 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).merge(doi: params[:id]) - puts options - if options[:provider_symbol] - doi = Doi.joins(:client, :provider).where(["dataset.doi = :doi and (allocator.symbol = :provider_symbol or dataset.aasm_state = 'findable')", options]).where(options).first - elsif options[:client_symbol] - doi = Doi.joins(:client).where(["dataset.doi = :doi and (datacentre.symbol = :client_symbol or dataset.aasm_state = 'findable')", options]).first - else - doi = Doi.where(options).first - end - - fail ActiveRecord::RecordNotFound unless doi.present? + # instead of using ability + doi = Doi.where(doi: params[:id]).first + fail ActiveRecord::RecordNotFound if not_allowed_by_doi_and_user(doi: doi, user: current_user) respond_to do |format| format.json do diff --git a/app/controllers/works_controller.rb b/app/controllers/works_controller.rb index 873deb577..cf271bd09 100644 --- a/app/controllers/works_controller.rb +++ b/app/controllers/works_controller.rb @@ -147,7 +147,7 @@ def set_doi bm = Benchmark.ms { @doi = Doi.where(doi: params[:id], aasm_state: "findable").first } - fail ActiveRecord::RecordNotFound unless @doi.present? + fail ActiveRecord::RecordNotFound if @doi.blank? logger.warn method: "GET", path: "/works/#{@doi.doi}", message: "Request DB /works/#{@doi.doi}", duration: bm end diff --git a/app/models/concerns/authenticable.rb b/app/models/concerns/authenticable.rb index fd61c4505..58fee0820 100644 --- a/app/models/concerns/authenticable.rb +++ b/app/models/concerns/authenticable.rb @@ -153,19 +153,15 @@ def secure_compare(a, b) res == 0 end - # filter results based on user permissions - def filter_doi_by_role(user) - return { aasm_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_symbol => user.provider_id.upcase } - elsif %w(client_admin client_user user temporary).include?(user.role_id) && user.client_id.present? - { :client_symbol => user.client_id.upcase } - else - { aasm_state: "findable" } - end + # check user permissions + def not_allowed_by_doi_and_user(doi: nil, user: nil) + return true if doi.blank? + return false if doi.aasm_state == "findable" + return true if user.blank? + return false if %w(staff_admin staff_user).include?(user.role_id) + return false if %w(provider_admin provider_user).include?(user.role_id) && user.provider_id.present? && user.provider_id == doi.provider_id + return false if %w(client_admin client_user user temporary).include?(user.role_id) && user.client_id.present? && user.client_id == doi.client_id + return true end end diff --git a/spec/concerns/authenticable_spec.rb b/spec/concerns/authenticable_spec.rb index 3e2031fdc..d13aca379 100644 --- a/spec/concerns/authenticable_spec.rb +++ b/spec/concerns/authenticable_spec.rb @@ -54,59 +54,123 @@ 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_symbol=>"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_symbol=>"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_symbol=>"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_symbol=>"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(:aasm_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(:aasm_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(:aasm_state=>"findable") + describe "not_allowed_by_doi_and_user" do + context "findable doi" do + let(:doi) { create(:doi, event: "publish") } + + it "staff_admin" do + token = User.generate_token(role_id: "staff_admin") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "staff_user" do + token = User.generate_token(role_id: "staff_user") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "provider_admin" do + token = User.generate_token(role_id: "provider_admin", provider_id: "datacite") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "provider_user" do + token = User.generate_token(role_id: "provider_user", provider_id: "datacite") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "client_admin" do + token = User.generate_token(role_id: "client_admin", client_id: "datacite.rph") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "client_user" do + token = User.generate_token(role_id: "client_user", client_id: "datacite.rph") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "user" do + token = User.generate_token(role_id: "user") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "temporary" do + token = User.generate_token(role_id: "temporary") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "anonymous" do + token = User.generate_token(role_id: "anonymous") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + end + + context "draft doi" do + let(:provider) { create(:provider, symbol: "DATACITE") } + let(:client) { create(:client, provider: provider, symbol: "DATACITE.RPH") } + let(:doi) { create(:doi, client: client) } + + it "staff_admin" do + token = User.generate_token(role_id: "staff_admin") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "staff_user" do + token = User.generate_token(role_id: "staff_user") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "provider_admin" do + token = User.generate_token(role_id: "provider_admin", provider_id: "datacite") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "provider_user" do + token = User.generate_token(role_id: "provider_user", provider_id: "datacite") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "client_admin" do + token = User.generate_token(role_id: "client_admin", client_id: "datacite.rph") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "client_user" do + token = User.generate_token(role_id: "client_user", client_id: "datacite.rph") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be false + end + + it "user" do + token = User.generate_token(role_id: "user") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be true + end + + it "temporary" do + token = User.generate_token(role_id: "temporary") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be true + end + + it "anonymous" do + token = User.generate_token(role_id: "anonymous") + subject = User.new(token) + expect(subject.not_allowed_by_doi_and_user(doi: doi, user: subject)).to be true + end end end diff --git a/spec/requests/dois_spec.rb b/spec/requests/dois_spec.rb index 97354a40b..889526117 100644 --- a/spec/requests/dois_spec.rb +++ b/spec/requests/dois_spec.rb @@ -77,6 +77,19 @@ end end + context 'provider_admin' do + let(:provider_bearer) { Client.generate_token(role_id: "provider_admin", uid: provider.symbol, provider_id: provider.symbol.downcase, password: provider.password) } + let(:provider_headers) { { 'HTTP_ACCEPT'=>'application/vnd.api+json', 'HTTP_AUTHORIZATION' => 'Bearer ' + provider_bearer }} + + + it 'returns the Doi' do + get "/dois/#{doi.doi}", nil, provider_headers + + expect(last_response.status).to eq(200) + expect(json.dig('data', 'attributes', 'doi')).to eq(doi.doi.downcase) + end + end + context 'anonymous user' do it 'returns the Doi' do get "/dois/#{doi.doi}"