Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to authorize the calling user to access an Application #1354

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ User-visible changes worth mentioning.

## master

- [#1354] Add option to authorize the calling user to access an application.
- [#1355] Allow to enable polymorphic Resource Owner association for Access Token & Grant
models (`use_polymorphic_resource_owner` configuration option).

Expand Down
6 changes: 5 additions & 1 deletion app/controllers/doorkeeper/authorizations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions lib/doorkeeper/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,10 @@ def configure_secrets_for(type, using:, fallback:)
default: {},
deprecated: { message: "Customize Doorkeeper models instead" }

# Hook to allow arbitrary user-client authorization
option :authorize_resource_owner_for_client,
default: ->(_client, _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
Expand Down
11 changes: 9 additions & 2 deletions lib/doorkeeper/oauth/pre_authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ class PreAuthorization

validate :client_id, error: :invalid_request
validate :client, error: :invalid_client
# The authorize_resource_owner_for_client config option is used for this validation
validate :access_to_client, error: :invalid_client
Copy link
Member

@nbulaj nbulaj Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And considering comment about the option name let rename it to resource_owner_authorized_for_client. Don't get angst about the long naming just because I think it must be explicit rather than implicit.

validate :redirect_uri, error: :invalid_redirect_uri
validate :params, error: :invalid_request
validate :response_type, error: :unsupported_response_type
Expand All @@ -15,9 +17,9 @@ class PreAuthorization
validate :client_supports_grant_flow, error: :unauthorized_client

attr_reader :server, :client_id, :client, :redirect_uri, :response_type, :state,
:code_challenge, :code_challenge_method, :missing_param
:code_challenge, :code_challenge_method, :missing_param, :resource_owner

def initialize(server, attrs = {})
def initialize(server, attrs = {}, resource_owner = nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK for me just because attrs not options, so we can add one more argument

@server = server
@client_id = attrs[:client_id]
@response_type = attrs[:response_type]
Expand All @@ -26,6 +28,7 @@ def initialize(server, attrs = {})
@state = attrs[:state]
@code_challenge = attrs[:code_challenge]
@code_challenge_method = attrs[:code_challenge_method]
@resource_owner = resource_owner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add it also to attr_readers, maybe we wanna to access it in the future for some reason

end

def authorizable?
Expand Down Expand Up @@ -137,6 +140,10 @@ def pre_auth_hash
status: I18n.t("doorkeeper.pre_authorization.status"),
}
end

def validate_access_to_client
client.application.authorized_for_resource_owner?(@resource_owner)
end
end
end
end
4 changes: 4 additions & 0 deletions lib/doorkeeper/orm/active_record/mixins/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ def as_json(options = {})
hash
end

def authorized_for_resource_owner?(resource_owner)
Doorkeeper.configuration.authorize_resource_owner_for_client.call(self, resource_owner)
end

private

def generate_uid
Expand Down
174 changes: 149 additions & 25 deletions spec/controllers/authorizations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,37 @@ def query_params
end
end

context "when user cannot access application" do
before do
allow(Doorkeeper.configuration).to receive(:authorize_resource_owner_for_client).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
Expand Down Expand Up @@ -214,6 +245,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(:authorize_resource_owner_for_client).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)
Expand Down Expand Up @@ -489,46 +553,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(:authorize_resource_owner_for_client).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(:authorize_resource_owner_for_client).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

Expand Down
9 changes: 9 additions & 0 deletions spec/lib/oauth/pre_authorization_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
response_type: "code",
redirect_uri: "https://app.com/callback",
state: "save-this",
current_resource_owner: Object.new,
}
end

Expand Down Expand Up @@ -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(:authorize_resource_owner_for_client).and_return(->(*_) { false }) }

it "is not authorizable" do
expect(subject).not_to be_authorizable
end
end

describe "as_json" do
before { subject.authorizable? }

Expand Down