From dcd7c1b3643be76949a243a4824aa2057e2aab0a Mon Sep 17 00:00:00 2001 From: JoshNorthrup Date: Sat, 25 Jan 2020 10:27:21 -0400 Subject: [PATCH] add option to define a resource owner's ability to access an application during pre-auth --- CHANGELOG.md | 1 + .../doorkeeper/authorizations_controller.rb | 6 +- lib/doorkeeper/config.rb | 3 + lib/doorkeeper/oauth/pre_authorization.rb | 8 +- .../orm/active_record/mixins/application.rb | 4 + .../authorizations_controller_spec.rb | 174 +++++++++++++++--- spec/lib/oauth/pre_authorization_spec.rb | 9 + 7 files changed, 178 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9caba0bf..8ca9064ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ User-visible changes worth mentioning. ## master +- [#1354] Add option to authorize the calling user to access an application. - [#1356] Invalidate duplicated scopes for Access Tokens and Grants in database. - [#1357] Fix `Doorkeeper::OAuth::PreAuthorization#as_json` method causing `Stack level too deep` error with AMS (fix #1312). diff --git a/app/controllers/doorkeeper/authorizations_controller.rb b/app/controllers/doorkeeper/authorizations_controller.rb index 7bb5518dd..b1d586f70 100644 --- a/app/controllers/doorkeeper/authorizations_controller.rb +++ b/app/controllers/doorkeeper/authorizations_controller.rb @@ -65,7 +65,11 @@ def redirect_or_render(auth) end def pre_auth - @pre_auth ||= OAuth::PreAuthorization.new(Doorkeeper.configuration, pre_auth_params) + @pre_auth ||= OAuth::PreAuthorization.new( + Doorkeeper.configuration, + pre_auth_params, + current_resource_owner, + ) end def pre_auth_params diff --git a/lib/doorkeeper/config.rb b/lib/doorkeeper/config.rb index cc7d04593..7e05feb73 100644 --- a/lib/doorkeeper/config.rb +++ b/lib/doorkeeper/config.rb @@ -274,6 +274,9 @@ def configure_secrets_for(type, using:, fallback:) default: {}, deprecated: { message: "Customize Doorkeeper models instead" } + # Hook to allow arbitrary user-application access enforcement + option :resource_owner_application_access, default: ->(_application, _resource_owner) { true } + # Allows to customize OAuth grant flows that +each+ application support. # You can configure a custom block (or use a class respond to `#call`) that must # return `true` in case Application instance supports requested OAuth grant flow diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index afdeb9cdf..533919f3f 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -7,6 +7,7 @@ class PreAuthorization validate :client_id, error: :invalid_request validate :client, error: :invalid_client + validate :access_to_client, error: :invalid_client validate :redirect_uri, error: :invalid_redirect_uri validate :params, error: :invalid_request validate :response_type, error: :unsupported_response_type @@ -17,7 +18,7 @@ class PreAuthorization attr_reader :server, :client_id, :client, :redirect_uri, :response_type, :state, :code_challenge, :code_challenge_method, :missing_param - def initialize(server, attrs = {}) + def initialize(server, attrs = {}, resource_owner = nil) @server = server @client_id = attrs[:client_id] @response_type = attrs[:response_type] @@ -26,6 +27,7 @@ def initialize(server, attrs = {}) @state = attrs[:state] @code_challenge = attrs[:code_challenge] @code_challenge_method = attrs[:code_challenge_method] + @resource_owner = resource_owner end def authorizable? @@ -137,6 +139,10 @@ def pre_auth_hash status: I18n.t("doorkeeper.pre_authorization.status"), } end + + def validate_access_to_client + client.application.viewable_to_resource_owner?(@resource_owner) + end end end end diff --git a/lib/doorkeeper/orm/active_record/mixins/application.rb b/lib/doorkeeper/orm/active_record/mixins/application.rb index cf34134c2..e6c8697c9 100644 --- a/lib/doorkeeper/orm/active_record/mixins/application.rb +++ b/lib/doorkeeper/orm/active_record/mixins/application.rb @@ -72,6 +72,10 @@ def as_json(options = {}) hash end + def viewable_to_resource_owner?(resource_owner) + Doorkeeper.configuration.resource_owner_application_access.call(self, resource_owner) + end + private def generate_uid diff --git a/spec/controllers/authorizations_controller_spec.rb b/spec/controllers/authorizations_controller_spec.rb index d3939c232..ca83ef4cd 100644 --- a/spec/controllers/authorizations_controller_spec.rb +++ b/spec/controllers/authorizations_controller_spec.rb @@ -136,6 +136,37 @@ def query_params end end + context "when user cannot access application" do + before do + allow(Doorkeeper.configuration).to receive(:resource_owner_application_access).and_return(->(*_) { false }) + post :create, params: { + client_id: client.uid, + response_type: "token", + redirect_uri: client.redirect_uri, + } + end + + let(:response_json_body) { JSON.parse(response.body) } + + it "renders 400 error" do + expect(response.status).to eq 401 + end + + it "includes error name" do + expect(response_json_body["error"]).to eq("invalid_client") + end + + it "includes error description" do + expect(response_json_body["error_description"]).to eq( + translated_error_message(:invalid_client), + ) + end + + it "does not issue any access token" do + expect(Doorkeeper::AccessToken.all).to be_empty + end + end + context "when other error happens" do before do default_scopes_exist :public @@ -207,6 +238,39 @@ def query_params end end + context "when user cannot access application" do + before do + allow(Doorkeeper.configuration).to receive(:api_only).and_return(true) + allow(Doorkeeper.configuration).to receive(:resource_owner_application_access).and_return(->(*_) { false }) + + post :create, params: { + client_id: client.uid, + response_type: "token", + redirect_uri: client.redirect_uri, + } + end + + let(:response_json_body) { JSON.parse(response.body) } + + it "renders 400 error" do + expect(response.status).to eq 401 + end + + it "includes error name" do + expect(response_json_body["error"]).to eq("invalid_client") + end + + it "includes error description" do + expect(response_json_body["error_description"]).to eq( + translated_error_message(:invalid_client), + ) + end + + it "does not issue any access token" do + expect(Doorkeeper::AccessToken.all).to be_empty + end + end + context "when other error happens" do before do allow(Doorkeeper.config).to receive(:api_only).and_return(true) @@ -482,46 +546,106 @@ def query_params end describe "GET #new with errors" do - before do - default_scopes_exist :public - get :new, params: { an_invalid: "request" } - end + context "without valid params" do + before do + default_scopes_exist :public + get :new, params: { an_invalid: "request" } + end + + it "does not redirect" do + expect(response).to_not be_redirect + end - it "does not redirect" do - expect(response).to_not be_redirect + it "does not issue any token" do + expect(Doorkeeper::AccessGrant.count).to eq 0 + expect(Doorkeeper::AccessToken.count).to eq 0 + end end - it "does not issue any token" do - expect(Doorkeeper::AccessGrant.count).to eq 0 - expect(Doorkeeper::AccessToken.count).to eq 0 + context "when user cannot access application" do + before do + allow(Doorkeeper.configuration).to receive(:resource_owner_application_access).and_return(->(*_) { false }) + + get :new, params: { + client_id: client.uid, + response_type: "token", + redirect_uri: client.redirect_uri, + } + end + + it "does not redirect" do + expect(response).to_not be_redirect + end + + it "does not issue any token" do + expect(Doorkeeper::AccessGrant.count).to eq 0 + expect(Doorkeeper::AccessToken.count).to eq 0 + end end end describe "GET #new in API mode with errors" do - let(:response_json_body) { JSON.parse(response.body) } - before do - default_scopes_exist :public allow(Doorkeeper.configuration).to receive(:api_only).and_return(true) - get :new, params: { an_invalid: "request" } + default_scopes_exist :public end - it "should render bad request" do - expect(response).to have_http_status(:bad_request) - end + context "without valid params" do + before do + get :new, params: { an_invalid: "request" } + end - it "includes error in body" do - expect(response_json_body["error"]).to eq("invalid_request") - end + let(:response_json_body) { JSON.parse(response.body) } + + it "should render bad request" do + expect(response).to have_http_status(:bad_request) + end + + it "includes error in body" do + expect(response_json_body["error"]).to eq("invalid_request") + end - it "includes error description in body" do - expect(response_json_body["error_description"]) - .to eq(translated_invalid_request_error_message(:missing_param, :client_id)) + it "includes error description in body" do + expect(response_json_body["error_description"]) + .to eq(translated_invalid_request_error_message(:missing_param, :client_id)) + end + + it "does not issue any token" do + expect(Doorkeeper::AccessGrant.count).to eq 0 + expect(Doorkeeper::AccessToken.count).to eq 0 + end end - it "does not issue any token" do - expect(Doorkeeper::AccessGrant.count).to eq 0 - expect(Doorkeeper::AccessToken.count).to eq 0 + context "when user cannot access application" do + before do + allow(Doorkeeper.configuration).to receive(:resource_owner_application_access).and_return(->(*_) { false }) + + get :new, params: { + client_id: client.uid, + response_type: "token", + redirect_uri: client.redirect_uri, + } + end + + let(:response_json_body) { JSON.parse(response.body) } + + it "should render bad request" do + expect(response).to have_http_status(:bad_request) + end + + it "includes error in body" do + expect(response_json_body["error"]).to eq("invalid_client") + end + + it "includes error description in body" do + expect(response_json_body["error_description"]) + .to eq(translated_error_message(:invalid_client)) + end + + it "does not issue any token" do + expect(Doorkeeper::AccessGrant.count).to eq 0 + expect(Doorkeeper::AccessToken.count).to eq 0 + end end end diff --git a/spec/lib/oauth/pre_authorization_spec.rb b/spec/lib/oauth/pre_authorization_spec.rb index 4ca9344f1..98d5f0122 100644 --- a/spec/lib/oauth/pre_authorization_spec.rb +++ b/spec/lib/oauth/pre_authorization_spec.rb @@ -19,6 +19,7 @@ response_type: "code", redirect_uri: "https://app.com/callback", state: "save-this", + current_resource_owner: Object.new, } end @@ -177,6 +178,14 @@ expect(subject).not_to be_authorizable end + context "when resource_owner cannot access client application" do + before { allow(Doorkeeper.configuration).to receive(:resource_owner_application_access).and_return(->(*_) { false }) } + + it "is not authorizable" do + expect(subject).not_to be_authorizable + end + end + describe "as_json" do before { subject.authorizable? }