Address issues that were detected during review

pull/10708/head
Wieland Lindenthal 2 years ago
parent fcf3f34899
commit 22defd8019
No known key found for this signature in database
GPG Key ID: 7ACCABE64832A0C6
  1. 2
      app/controllers/oauth_clients_controller.rb
  2. 27
      app/services/oauth_clients/connection_manager.rb
  3. 4
      lib/api/v3/utilities/path_helper.rb
  4. 32
      spec/requests/oauth_clients/callback_flow_spec.rb

@ -128,7 +128,7 @@ class OAuthClientsController < ApplicationController
# This needs to be modified as soon as we support more integration types.
if User.current.admin && state && nextcloud?
yield
elsif state
elsif ::API::V3::Utilities::PathHelper::ApiV3Path::same_origin? state
flash[:error] = [t(:'oauth_client.errors.oauth_issue_contact_admin')]
redirect_to state
else

@ -87,6 +87,10 @@ module OAuthClients
# In the current implementation "state" just consists of the URL of
# the initial page, possibly with "&var=value" added parameters.
# So we can just return this URI.
unless ::API::V3::Utilities::PathHelper::ApiV3Path::same_origin?(state)
return ::API::V3::Utilities::PathHelper::ApiV3Path::root_url
end
state
end
@ -137,9 +141,9 @@ module OAuthClients
# Localize the error message
def i18n_rack_oauth2_error_message(rack_oauth2_client_exception)
l10n_key = "oauth_client.errors.rack_oauth2.#{rack_oauth2_client_exception.message}"
if I18n.exists? l10n_key
I18n.t(l10n_key)
i18n_key = "oauth_client.errors.rack_oauth2.#{rack_oauth2_client_exception.message}"
if I18n.exists? i18n_key
I18n.t(i18n_key)
else
"#{I18n.t('oauth_client.errors.oauth_returned_error')}: #{rack_oauth2_client_exception.message.to_html}"
end
@ -148,12 +152,22 @@ module OAuthClients
# Return a fully configured RackOAuth2Client.
# This client does all the heavy lifting with the OAuth2 protocol.
def rack_oauth_client(options = {})
rack_oauth_client = build_basic_rack_oauth_client
# Write options, for example authorization_code and refresh_token
rack_oauth_client.refresh_token = options[:refresh_token] if options[:refresh_token]
rack_oauth_client.authorization_code = options[:authorization_code] if options[:authorization_code]
rack_oauth_client
end
def build_basic_rack_oauth_client
oauth_client_uri = URI.parse(@oauth_client.integration.host)
oauth_client_scheme = oauth_client_uri.scheme
oauth_client_host = oauth_client_uri.host
oauth_client_port = oauth_client_uri.port
client = Rack::OAuth2::Client.new(
Rack::OAuth2::Client.new(
identifier: @oauth_client.client_id,
secret: @oauth_client.client_secret,
scheme: oauth_client_scheme,
@ -162,11 +176,6 @@ module OAuthClients
authorization_endpoint: "/apps/oauth2/authorize",
token_endpoint: "/apps/oauth2/api/v1/token"
)
# Write options, for example authorization_code and refresh_token
client.refresh_token = options[:refresh_token] if options[:refresh_token]
client.authorization_code = options[:authorization_code] if options[:authorization_code]
client
end
# Create a new OpenProject token object based on the return values

@ -92,6 +92,10 @@ module API
"#{root_path}api/v3"
end
def self.same_origin?(url)
url.to_s.start_with? root_url
end
index :action
show :action

@ -39,7 +39,9 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do
"mBf4v9hNA6hXXCWHd5mZggsAa2FSOXinx9jKx1yjSoDwOPOX4k6zGEgM2radqgg1nRwXCqvIe5xZsfwqMIaTdL" +
"jYnl0OpYOc6ePblzQTmnlp7RYiHW09assYEJjv9zps"
end
let(:state) { "https://example.org/my-path?and=some&query=params" }
let(:state) do
File.join(::API::V3::Utilities::PathHelper::ApiV3Path::root_url, "/my-path?and=some&query=params")
end
let(:oauth_client_token) { create :oauth_client_token }
let(:oauth_client) do
create :oauth_client,
@ -57,7 +59,6 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do
subject(:response) { last_response }
before do
host! 'https://my-example.org'
login_as current_user
allow(::Rack::OAuth2::Client).to receive(:new).and_return(rack_oauth2_client)
@ -84,6 +85,13 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do
end
end
shared_examples 'fallback redirect' do
it 'redirects to home' do
expect(response.status).to eq 302
expect(URI(response.location).path).to eq home_path
end
end
context 'with valid params' do
context 'without errors' do
before do
@ -103,6 +111,19 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do
end
end
context 'with a state param containing a URL pointing to a different host' do
let(:state) { "https://some-other-domain.com/foo/bar" }
before do
uri.query = URI.encode_www_form([['code', code], ['state', state]])
get uri.to_s
subject
end
it_behaves_like 'fallback redirect'
end
context 'with some other error, having a state param' do
before do
allow(::OAuthClients::ConnectionManager)
@ -136,7 +157,7 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do
subject
end
context 'with current_user being not being an admin' do
context 'with current_user not being an admin' do
it_behaves_like 'with errors and state param, not being admin'
end
@ -155,9 +176,6 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do
subject
end
it 'redirects to home' do
expect(response.status).to eq 302
expect(URI(response.location).path).to eq home_path
end
it_behaves_like 'fallback redirect'
end
end

Loading…
Cancel
Save