From fcf3f34899af6c02673f3b47d50e065dd2a2c72c Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Mon, 23 May 2022 17:26:03 +0200 Subject: [PATCH 1/4] Adding ::OAuthClients::ConnectionManager and callback endpoint --- app/controllers/oauth_clients_controller.rb | 144 ++++++++ app/models/oauth_client_token.rb | 41 +++ .../oauth_clients/connection_manager.rb | 214 +++++++++++ config/locales/en.yml | 40 +++ config/routes.rb | 5 + ...220518154147_create_oauth_client_tokens.rb | 18 + .../storages/admin/storages/show.html.erb | 3 + spec/factories/oauth_client_token_factory.rb | 36 ++ spec/models/oauth_client_token_spec.rb | 111 ++++++ .../oauth_clients/callback_flow_spec.rb | 163 +++++++++ .../oauth_clients/connection_manager_spec.rb | 334 ++++++++++++++++++ 11 files changed, 1109 insertions(+) create mode 100644 app/controllers/oauth_clients_controller.rb create mode 100644 app/models/oauth_client_token.rb create mode 100644 app/services/oauth_clients/connection_manager.rb create mode 100644 db/migrate/20220518154147_create_oauth_client_tokens.rb create mode 100644 spec/factories/oauth_client_token_factory.rb create mode 100644 spec/models/oauth_client_token_spec.rb create mode 100644 spec/requests/oauth_clients/callback_flow_spec.rb create mode 100644 spec/services/oauth_clients/connection_manager_spec.rb diff --git a/app/controllers/oauth_clients_controller.rb b/app/controllers/oauth_clients_controller.rb new file mode 100644 index 000000000000..f8f7425b0659 --- /dev/null +++ b/app/controllers/oauth_clients_controller.rb @@ -0,0 +1,144 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# This controller handles OAuth2 Authorization Code Grant redirects from a Authorization Server to +# "callback" endpoint. +class OAuthClientsController < ApplicationController + before_action :find_oauth_client + before_action :set_state + before_action :set_code + + # Provide the OAuth2 "callback" endpoint. + # The Authorization Server redirects + # here after successful authentication and authorization. + # This endpoint gets a "code" parameter that cryptographically + # contains a grant. + # We get here by a URL like this: + # http://localhost:4200/oauth_clients/asdf12341234qsdfasdfasdf/callback? + # state=http%3A%2F%2Flocalhost%3A4200%2Fprojects%2Fdemo-project%2Foauth2_example& + # code=MQoOnUTJGFdAo5jBGD1SqnDH0PV6yioG7NoYM2zZZlK3g6LuKrGUmOxjIS1bIy7fHEfZy2WrgYcx + def callback + connection_manager = OAuthClients::ConnectionManager.new(user: User.current, oauth_client: @oauth_client) + + # Exchange the code with a token using a HTTP call to the Authorization Server + service_result = connection_manager.code_to_token(@code) + + if service_result.success? + # Redirect the user to the page that initially wanted to access the OAuth2 resource. + # "state" is a variable that encapsulates the page's URL and status. + redirect_uri = connection_manager.callback_redirect_uri(@state) + redirect_to redirect_uri + else + # We got a list of errors from ::OAuthClients::ConnectionManager + set_oauth_errors(service_result) + + redirect_user_or_admin(@state) do + # If the current user is an admin, we send her directly to the + # settings that she needs to edit. + redirect_to admin_settings_storage_path(@oauth_client.integration) + end + end + end + + private + + def set_oauth_errors(service_result) + flash[:error] = ["#{t(:'oauth_client.errors.oauth_authorization_code_grant_had_errors')}:"] + service_result.errors.each do |error| + flash[:error] << "#{t(:'oauth_client.errors.oauth_reported')}: #{error.full_message}" + end + end + + def set_code + # The OAuth2 provider should have sent a code when using response_type = "code" + # So this could either be an error from the Authorization Server (i.e. Nextcloud) or + # ::OAuthClient::ConnectionManager has used the wrong response_type. + @code = params[:code] + if @code.blank? + flash[:error] = [I18n.t('oauth_client.errors.oauth_code_not_present'), + I18n.t('oauth_client.errors.oauth_code_not_present_explanation')] + + redirect_user_or_admin params[:state] do + # If the current user is an admin, we send her directly to the + # settings that she needs to edit. + redirect_to admin_settings_storage_path(@oauth_client.integration) + end + end + end + + def set_state + # state is used by OpenProject to contain the redirection URL where to + # continue after receiving an OAuth2 access token. So it should not be blank. + @state = params[:state] + if @state.blank? + flash[:error] = [I18n.t('oauth_client.errors.oauth_state_not_present'), + I18n.t('oauth_client.errors.oauth_state_not_present_explanation')] + + redirect_user_or_admin(nil) do + # If the current user is an admin, we send her directly to the + # settings that she needs to edit. + redirect_to admin_settings_storage_path(@oauth_client.integration) + end + end + end + + def find_oauth_client + @oauth_client = OAuthClient.find_by(client_id: params[:oauth_client_id]) + if @oauth_client.nil? + # oauth_client can be nil if OAuthClient was not found. + # This happens during admin setup if the user forgot to update the return_uri + # on the Authorization Server (i.e. Nextcloud) after updating the OpenProject + # side with a new client_id and client_secret. + flash[:error] = [I18n.t('oauth_client.errors.oauth_client_not_found'), + I18n.t('oauth_client.errors.oauth_client_not_found_explanation')] + + redirect_user_or_admin params[:state] do + # Something must be wrong in the storage's setup + redirect_to admin_settings_storages_path + end + end + end + + def redirect_user_or_admin(state = nil) + # This needs to be modified as soon as we support more integration types. + if User.current.admin && state && nextcloud? + yield + elsif state + flash[:error] = [t(:'oauth_client.errors.oauth_issue_contact_admin')] + redirect_to state + else + redirect_to home_path + end + end + + def nextcloud? + @oauth_client&.integration && \ + @oauth_client.integration.is_a?(::Storages::Storage) && \ + @oauth_client.integration.provider_type == 'nextcloud' + end +end diff --git a/app/models/oauth_client_token.rb b/app/models/oauth_client_token.rb new file mode 100644 index 000000000000..bc19751ab958 --- /dev/null +++ b/app/models/oauth_client_token.rb @@ -0,0 +1,41 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# OAuthClientToken stores the OAuth2 Bearer+Refresh tokens that +# an OAuth2 server (Nextcloud or similar) provides after a user +# has granted access. +class OAuthClientToken < ApplicationRecord + # OAuthClientToken sits between User and OAuthClient + belongs_to :user, optional: false + belongs_to :oauth_client, optional: false + + validates :user, uniqueness: { scope: :oauth_client } + + validates :access_token, length: { minimum: 1, maximum: 255 } + validates :refresh_token, length: { minimum: 1, maximum: 255 } +end diff --git a/app/services/oauth_clients/connection_manager.rb b/app/services/oauth_clients/connection_manager.rb new file mode 100644 index 000000000000..8e1c7379842d --- /dev/null +++ b/app/services/oauth_clients/connection_manager.rb @@ -0,0 +1,214 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "rack/oauth2" +require "uri/http" + +module OAuthClients + class ConnectionManager + attr_reader :user, :oauth_client + + def initialize(user:, oauth_client:) + @user = user + @oauth_client = oauth_client + end + + # Main method to initiate the OAuth2 flow called by a "client" component + # that wants to access OAuth2 protected resources. + # Returns an OAuthClientToken object or a String in case a renew is required. + # @param state (OAuth2 RFC) encapsulates the state of the calling page (URL + params) to return + # @param scope (OAuth2 RFC) specifies the resources to access. Nextcloud only has one global scope. + def get_access_token(scope: [], state: nil) + # Check for an already existing token from last call + token = get_existing_token + return ServiceResult.new(success: true, result: token) if token.present? + + # Return a String with a redirect URL to Nextcloud instead of a token + @redirect_url = redirect_to_oauth_authorize(scope:, state:) + ServiceResult.new(success: false, result: @redirect_url) + end + + # The bearer/access token has expired or is due for renew for other reasons. + # Talk to OAuth2 Authorization Server to exchange the renew_token for a new bearer token. + def refresh_token + # There should already be an existing token, + # otherwise this method has been called too early (internal flow error). + oauth_client_token = get_existing_token + if oauth_client_token.nil? + return service_result_with_error(I18n.t('oauth_client.errors.refresh_token_called_without_existing_token')) + end + + # Get the Rack::OAuth2::Client and call access_token!, then return a ServiceResult. + service_result = request_new_token(refresh_token: oauth_client_token.refresh_token) + return service_result unless service_result.success? + + # Updated tokens, handle model checking errors and return a ServiceResult + update_oauth_client_token(oauth_client_token, service_result.result) + end + + # Redirect to the "authorize" endpoint of the OAuth2 Authorization Server. + # @param state (OAuth2 RFC) encapsulates the state of the calling page (URL + params) to return + # @param scope (OAuth2 RFC) specifies the resources to access. Nextcloud only has one global scope. + def redirect_to_oauth_authorize(scope: [], state: nil) + client = rack_oauth_client # Configure and start the rack-oauth2 client + client.authorization_uri(scope:, state:) + end + + # For the OAuth2 callback page: Calculate the redirection URL that will + # point the browser at the initial page that wanted to access the OAuth2 + # protected resource. + # @param state (OAuth2 RFC) encapsulates the state of the calling page (URL + params) to return + def callback_redirect_uri(state) + # 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. + state + end + + # Called by callback_page with a cryptographic "code" that indicates + # that the user has successfully authorized the OAuth2 Authorization Server. + # We now are going to exchange this code to a token (bearer+refresh) + def code_to_token(code) + # Return a Rack::OAuth2::AccessToken::Bearer or an error string + service_result = request_new_token(authorization_code: code) + return service_result unless service_result.success? + + # Create a new OAuthClientToken from Rack::OAuth::AccessToken::Bearer and return + ServiceResult.new( + success: true, + result: create_new_oauth_client_token(service_result.result) + ) + end + + private + + # Check if a OAuthClientToken already exists and return nil otherwise. + # Don't handle the case of an expired token. + def get_existing_token + # Check if we've got a token in the database and return nil otherwise. + OAuthClientToken.find_by(user_id: @user, oauth_client_id: @oauth_client.id) + end + + # Calls client.access_token! + # Convert the various exceptions into user-friendly error strings. + def request_new_token(options = {}) + rack_access_token = rack_oauth_client(options) + .access_token!(:body) # Rack::OAuth2::AccessToken + + ServiceResult.new(success: true, + result: rack_access_token) + rescue Rack::OAuth2::Client::Error => e # Handle Rack::OAuth2 specific errors + service_result_with_error(i18n_rack_oauth2_error_message(e)) + rescue Timeout::Error, EOFError, Net::HTTPBadResponse, Net::HTTPHeaderSyntaxError, Net::ProtocolError, + Errno::EINVAL, Errno::ENETUNREACH, Errno::ECONNRESET, Errno::ECONNREFUSED, JSON::ParserError => e + service_result_with_error( + "#{I18n.t('oauth_client.errors.oauth_returned_http_error')}: #{e.class}: #{e.message.to_html}" + ) + rescue StandardError => e + service_result_with_error( + "#{I18n.t('oauth_client.errors.oauth_returned_standard_error')}: #{e.class}: #{e.message.to_html}" + ) + end + + # 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) + else + "#{I18n.t('oauth_client.errors.oauth_returned_error')}: #{rack_oauth2_client_exception.message.to_html}" + end + end + + # Return a fully configured RackOAuth2Client. + # This client does all the heavy lifting with the OAuth2 protocol. + def rack_oauth_client(options = {}) + 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( + identifier: @oauth_client.client_id, + secret: @oauth_client.client_secret, + scheme: oauth_client_scheme, + host: oauth_client_host, + port: oauth_client_port, + 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 + # from a Rack::OAuth2::AccessToken::Bearer token + def create_new_oauth_client_token(rack_access_token) + OAuthClientToken.create( + user: @user, + oauth_client: @oauth_client, + origin_user_id: rack_access_token.raw_attributes[:user_id], # ID of user at OAuth2 Authorization Server + access_token: rack_access_token.access_token, + token_type: rack_access_token.token_type, # :bearer + refresh_token: rack_access_token.refresh_token, + expires_in: rack_access_token.raw_attributes[:expires_in], + scope: rack_access_token.scope + ) + end + + # Update an OpenProject token based on updated values from a + # Rack::OAuth2::AccessToken::Bearer after a OAuth2 refresh operation + def update_oauth_client_token(oauth_client_token, rack_oauth2_access_token) + success = oauth_client_token.update( + access_token: rack_oauth2_access_token.access_token, + refresh_token: rack_oauth2_access_token.refresh_token, + expires_in: rack_oauth2_access_token.expires_in + ) + + if success + ServiceResult.new(success: true, result: oauth_client_token) + else + result = ServiceResult.new(success: false) + result.errors.add(:base, I18n.t('oauth_client.errors.refresh_token_updated_failed')) + result.add_dependent!(ServiceResult.new(success: false, errors: oauth_client_token.errors)) + result + end + end + + # Shortcut method to convert an error message into an unsuccessful + # ServiceResult with that error message + def service_result_with_error(message) + ServiceResult.new(success: false).tap do |result| + result.errors.add(:base, message) + end + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index e6fc01091f05..fa8f2d25b722 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3213,4 +3213,44 @@ en: revoke_my_application_confirmation: "Do you really want to remove this application? This will revoke %{token_count} active for it." my_registered_applications: "Registered OAuth applications" + oauth_client: + labels: + label_oauth_integration: "OAuth2 integration" + label_redirect_uri: "Redirect URI" + label_request_token: "Request token" + label_refresh_token: "Refresh token" + errors: + oauth_authorization_code_grant_had_errors: "OAuth2 returned an error" + oauth_reported: "OAuth2 provider reported" + oauth_returned_error: "OAuth2 returned an error" + oauth_returned_json_error: "OAuth2 returned a JSON error" + oauth_returned_http_error: "OAuth2 returned a network error" + oauth_returned_standard_error: "OAuth2 returned an internal error" + wrong_token_type_returned: "OAuth2 returned a wrong type of token, expecting AccessToken::Bearer" + oauth_issue_contact_admin: "OAuth2 reported an error. Please contact your system administrator." + oauth_client_not_found: "OAuth2 client not found in 'callback' endpoint (redirect_uri)." + refresh_token_called_without_existing_token: > + Internal error: Called refresh_token without a previously existing token. + refresh_token_updated_failed: "Error during update of OAuthClientToken" + oauth_client_not_found_explanation: > + This error appears after you have updated the client_id and client_secret + in OpenProject, but haven't updated the 'Return URI' field in the OAuth2 provider. + oauth_code_not_present: "OAuth2 'code' not found in 'callback' endpoint (redirect_uri)." + oauth_code_not_present_explanation: > + This error appears if you have selected the wrong response_type + in the OAuth2 provider. Response_type should be 'code' or similar. + oauth_state_not_present: "OAuth2 'state' not found in 'callback' endpoint (redirect_uri)." + oauth_state_not_present_explanation: > + The 'state' is used to indicate to OpenProject where to continue + after a successful OAuth2 authentication. + A missing 'state' is an internal error that may appear during setup. + Please contact your system administrator. + rack_oauth2: + client_secret_invalid: "Client secret is invalid" + invalid_request: > + OAuth2 server responded with 'invalid_request'. + This error appears if you try to authorize multiple times. + invalid_response: "OAuth2 server provided an invalid response" + + you: you diff --git a/config/routes.rb b/config/routes.rb index dee08a838f38..408f9c2d9823 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -579,6 +579,11 @@ get '(/*state)', to: 'angular#notifications_layout', as: :notifications_center end + # OAuthClient needs a "callback" URL that Nextcloud calls with a "code" (see OAuth2 RFC) + scope 'oauth_clients/:oauth_client_id' do + get 'callback', controller: 'oauth_clients', action: :callback + end + # Routes for design related documentation and examples pages get '/design/spot', to: 'angular#empty_layout' get '/design/styleguide' => redirect('/assets/styleguide.html') diff --git a/db/migrate/20220518154147_create_oauth_client_tokens.rb b/db/migrate/20220518154147_create_oauth_client_tokens.rb new file mode 100644 index 000000000000..d1bc03293a14 --- /dev/null +++ b/db/migrate/20220518154147_create_oauth_client_tokens.rb @@ -0,0 +1,18 @@ +class CreateOAuthClientTokens < ActiveRecord::Migration[6.1] + def change + create_table :oauth_client_tokens do |t| + t.references :oauth_client, null: false, foreign_key: { to_table: :oauth_clients, on_delete: :cascade } + t.references :user, null: false, index: true, foreign_key: { to_table: :users, on_delete: :cascade } + + t.string :access_token + t.string :refresh_token + t.string :token_type + t.integer :expires_in + t.string :scope + t.string :origin_user_id # ID of the current user on the _OAuth2_provider_side_ + + t.timestamps + t.index %i[user_id oauth_client_id], unique: true + end + end +end diff --git a/modules/storages/app/views/storages/admin/storages/show.html.erb b/modules/storages/app/views/storages/admin/storages/show.html.erb index 3f2225b56e24..cafa8894dec7 100644 --- a/modules/storages/app/views/storages/admin/storages/show.html.erb +++ b/modules/storages/app/views/storages/admin/storages/show.html.erb @@ -56,6 +56,7 @@ See COPYRIGHT and LICENSE files for more details.

<%= t(:label_general) %>

+
<%= t(:'storages.label_name') %>
@@ -82,6 +83,7 @@ See COPYRIGHT and LICENSE files for more details. <%= @object.creator.name %>
+
<%= Storages::ProjectStorage.human_attribute_name(:created_at) %>
@@ -90,6 +92,7 @@ See COPYRIGHT and LICENSE files for more details.
+
diff --git a/spec/factories/oauth_client_token_factory.rb b/spec/factories/oauth_client_token_factory.rb new file mode 100644 index 000000000000..543b4668d531 --- /dev/null +++ b/spec/factories/oauth_client_token_factory.rb @@ -0,0 +1,36 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +FactoryBot.define do + factory :oauth_client_token, class: '::OAuthClientToken' do + sequence(:access_token) { |n| "1234567890-#{n}" } + sequence(:refresh_token) { |n| "2345678901-#{n}" } + oauth_client factory: :oauth_client + user factory: :user + end +end diff --git a/spec/models/oauth_client_token_spec.rb b/spec/models/oauth_client_token_spec.rb new file mode 100644 index 000000000000..8e9d6034109d --- /dev/null +++ b/spec/models/oauth_client_token_spec.rb @@ -0,0 +1,111 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require 'spec_helper' + +describe OAuthClientToken, type: :model do + let(:access_token) { "x" } + let(:refresh_token) { "x" } + let(:user) { create :user } + let(:oauth_client) { create :oauth_client } + let(:instance) { described_class.new(access_token:, refresh_token:, user:, oauth_client:) } + + describe '#valid?' do + subject { instance.valid? } + + context 'with default arguments' do + it 'succeeds' do + expect(subject).to be_truthy + end + end + + context 'with access_token too long' do + let(:access_token) { "x" * 257 } + + it 'fails with access_token too long' do + expect(subject).to be_falsey + end + end + + context 'with refresh_token too long' do + let(:refresh_token) { "x" * 257 } + + it 'fails with refresh_token too long' do + expect(subject).to be_falsey + end + end + + context 'with access_token too short' do + let(:access_token) { "" } + + it 'fails with access_token too short' do + expect(subject).to be_falsey + end + end + + context 'with refresh_token too short' do + let(:refresh_token) { "" } + + it 'fails with refresh_token too short' do + expect(subject).to be_falsey + end + end + + context 'without access_token' do + let(:access_token) { nil } + + it 'fails with access_token is nil' do + expect(subject).to be_falsey + end + end + + context 'without refresh_token' do + let(:refresh_token) { nil } + + it 'fails with refresh_token is nil' do + expect(subject).to be_falsey + end + end + + context 'with invalid user' do + let(:user) { nil } + + it 'fails with invalid user' do + expect(subject).to be_falsey + end + end + + context 'with invalid oauth_client' do + let(:oauth_client) { nil } + + it 'fails with invalid oauth_client' do + expect(subject).to be_falsey + end + end + end +end diff --git a/spec/requests/oauth_clients/callback_flow_spec.rb b/spec/requests/oauth_clients/callback_flow_spec.rb new file mode 100644 index 000000000000..47053ea0b973 --- /dev/null +++ b/spec/requests/oauth_clients/callback_flow_spec.rb @@ -0,0 +1,163 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require 'spec_helper' +require 'rack/test' + +describe 'OAuthClient callback endpoint', :enable_storages, type: :request do + include Rack::Test::Methods + include API::V3::Utilities::PathHelper + + let(:current_user) { create(:user) } + + let(:code) do + "mBf4v9hNA6hXXCWHd5mZggsAa2FSOXinx9jKx1yjSoDwOPOX4k6zGEgM2radqgg1nRwXCqvIe5xZsfwqMIaTdL" + + "jYnl0OpYOc6ePblzQTmnlp7RYiHW09assYEJjv9zps" + end + let(:state) { "https://example.org/my-path?and=some&query=params" } + let(:oauth_client_token) { create :oauth_client_token } + let(:oauth_client) do + create :oauth_client, + client_id: 'kETWr2XsjPxhVbN7Q5jmPq83xribuUTRzgfXthpYT0vSqyJWm4dOnivKzHiZasf0', + client_secret: 'J1sg4L5PYbM2RZL3pUyxTnamvfpcP5eUcCPmeCQHJO60Gy6CJIdDaF4yXOeC8BPS' + end + let(:rack_oauth2_client) do + instance_double(::Rack::OAuth2::Client) + end + let(:connection_manager) do + instance_double(::OAuthClients::ConnectionManager) + end + let(:uri) { URI(File.join('oauth_clients', oauth_client.client_id, 'callback')) } + + 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) + allow(rack_oauth2_client) + .to receive(:access_token!).with(:body) + .and_return( + ::Rack::OAuth2::AccessToken::Bearer.new(access_token: 'xyzaccesstoken', + refresh_token: 'xyzrefreshtoken') + ) + allow(rack_oauth2_client).to receive(:authorization_code=) + end + + shared_examples 'with errors and state param, not being admin' do + it 'redirects to URI encode in state' do + expect(response.status).to eq 302 + expect(response.location).to eq state + end + end + + shared_examples 'with errors, being an admin' do + it 'redirects to admin settings for the storage' do + expect(response.status).to eq 302 + expect(URI(response.location).path).to eq admin_settings_storage_path(oauth_client.integration) + end + end + + context 'with valid params' do + context 'without errors' do + before do + uri.query = URI.encode_www_form([['code', code], ['state', state]]) + get uri.to_s + + subject + end + + it 'redirects to the URL that was provided by the state param' do + expect(rack_oauth2_client).to have_received(:authorization_code=).with(code) + expect(response.status).to eq 302 + expect(response.location).to eq state + expect(::OAuthClientToken.count).to eq 1 + expect(::OAuthClientToken.last.access_token).to eq 'xyzaccesstoken' + expect(::OAuthClientToken.last.refresh_token).to eq 'xyzrefreshtoken' + end + end + + context 'with some other error, having a state param' do + before do + allow(::OAuthClients::ConnectionManager) + .to receive(:new).and_return(connection_manager) + allow(connection_manager) + .to receive(:code_to_token).with(code).and_return(ServiceResult.new(success: false)) + + uri.query = URI.encode_www_form([['code', code], ['state', state]]) + get uri.to_s + + subject + end + + context 'with current_user being an admin' do + let(:current_user) { create :admin } + + it_behaves_like 'with errors, being an admin' + end + + context 'with current_user not being an admin' do + it_behaves_like 'with errors and state param, not being admin' + end + end + end + + context 'without code param, but with state param,' do + before do + uri.query = URI.encode_www_form([['state', state]]) + get uri.to_s + + subject + end + + context 'with current_user being not being an admin' do + it_behaves_like 'with errors and state param, not being admin' + end + + context 'with current_user being an admin' do + let(:current_user) { create :admin } + + it_behaves_like 'with errors, being an admin' + end + end + + context 'without state param' do + before do + uri.query = URI.encode_www_form([['code', code]]) + get uri.to_s + + subject + end + + it 'redirects to home' do + expect(response.status).to eq 302 + expect(URI(response.location).path).to eq home_path + end + end +end diff --git a/spec/services/oauth_clients/connection_manager_spec.rb b/spec/services/oauth_clients/connection_manager_spec.rb new file mode 100644 index 000000000000..f40b29c94d65 --- /dev/null +++ b/spec/services/oauth_clients/connection_manager_spec.rb @@ -0,0 +1,334 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require 'spec_helper' +require 'webmock/rspec' + +describe ::OAuthClients::ConnectionManager, type: :model do + let(:user) { create :user } + let(:host) { "http://example.org" } + let(:provider_type) { ::Storages::Storage::PROVIDER_TYPE_NEXTCLOUD } + let(:storage) { create(:storage, provider_type:, host: "#{host}/") } + let(:scope) { [:all] } # OAuth2 resources to access, specific to provider + let(:oauth_client) do + create(:oauth_client, + client_id: "nwz34rWsolvJvchfQ1bVHXfMb1ETK89lCBgzrLhWx3ACW5nKfmdcyf5ftlCyKGbk", + client_secret: "A08n6CRBOOr41iqkWRynnP6BbmEnau7LeP9t9xrIbiYX46iXgmIZgqhJoDFjUMEq", + integration: storage) + end + let(:oauth_client_token) { create(:oauth_client_token, oauth_client:, user:) } + let(:instance) { described_class.new(user:, oauth_client:) } + + # Test the redirect_to_oauth_authorize function that puts together + # the OAuth2 provider URL (Nextcloud) according to RFC specs. + describe '#redirect_to_oauth_authorize' do + let(:scope) { nil } + let(:state) { nil } + + subject { instance.redirect_to_oauth_authorize(scope:, state:) } + + context 'with empty state and scope' do + it 'returns the redirect URL' do + expect(subject).to be_a String + expect(subject).to include oauth_client.integration.host + expect(subject).not_to include "scope" + expect(subject).not_to include "state" + end + end + + context 'with state but empty scope' do + let(:state) { "https://example.com/page" } + + it 'returns the redirect URL' do + expect(subject).to be_a String + expect(subject).to include oauth_client.integration.host + expect(subject).not_to include "scope" + expect(subject).to include "&state=https" + end + end + + context 'with multiple scopes but empty state' do + let(:scope) { %i(email profile) } + + it 'returns the redirect URL' do + expect(subject).to be_a String + expect(subject).to include oauth_client.integration.host + expect(subject).not_to include "state" + expect(subject).to include "&scope=email%20profile" + end + end + end + + # The first step in the OAuth2 flow is to produce a URL for the + # user to authenticate and authorize access at the OAuth2 provider + # (Nextcloud). + describe '#get_access_token' do + subject { instance.get_access_token } + + context 'with no OAuthClientToken present' do + it 'returns a redirection URL' do + expect(subject.success).to be_falsey + expect(subject.result).to be_a String + # Details of string are tested above in section #redirect_to_oauth_authorize + end + end + + context 'with no OAuthClientToken present and state parameters' do + subject { instance.get_access_token(state: "some_state", scope: [:email]) } + + it 'returns the redirect URL' do + expect(subject.success).to be_falsey + expect(subject.result).to be_a String + expect(subject.result).to include oauth_client.integration.host + expect(subject.result).to include "&state=some_state" + expect(subject.result).to include "&scope=email" + end + end + + context 'with an OAuthClientToken present' do + before do + oauth_client_token + end + + it 'returns the OAuthClientToken' do + expect(subject).to be_truthy + expect(subject.result).to be_a OAuthClientToken # The one and only... + expect(subject.result).to eql oauth_client_token + end + end + end + + # In the second step the Authorization Server (Nextcloud) redirects + # to a "callback" endpoint on the OAuth2 client (OpenProject): + # http://:4200/oauth_clients/8/callback?state=&code=7kRGJ...jG3KZ + # This callback code basically just calls code_to_token(code). + # The callback endpoint calls code_to_token(code) with the code + # received and exchanges the code for a bearer+refresh token + # using a HTTP request. + describe '#code_to_token' do + let(:code) { "7kRGJ...jG3KZ" } + + subject { instance.code_to_token(code) } + + context 'with happy path' do + before do + # Simulate a successful authorization returning the tokens + response_body = { + access_token: "yjTDZ...RYvRH", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "UwFp...1FROJ", + user_id: "admin" + }.to_json + stub_request(:any, File.join(host, '/apps/oauth2/api/v1/token')) + .to_return(status: 200, body: response_body) + end + + it 'returns a valid ClientToken object', webmock: true do + expect(subject.success).to be_truthy + expect(subject.result).to be_a OAuthClientToken + end + end + + context 'with known reply invalid_request', webmock: true do + before do + stub_request(:post, File.join(host, '/apps/oauth2/api/v1/token')) + .to_return(status: 400, body: { error: "invalid_request" }.to_json) + end + + it 'returns a specific error message' do + expect(subject.success).to be_falsey + expect(subject.result).to be_nil + expect(subject.errors[:base].count).to be(1) + expect(subject.errors[:base].first).to include I18n.t('oauth_client.errors.rack_oauth2.invalid_request') + expect(subject.errors[:base].first).not_to include I18n.t('oauth_client.errors.oauth_returned_error') + end + end + + context 'with unknown reply', webmock: true do + before do + stub_request(:post, File.join(host, '/apps/oauth2/api/v1/token')) + .to_return(status: 400, body: { error: "invalid_requesttt" }.to_json) + end + + it 'returns an unspecific error message' do + expect(subject.success).to be_falsey + expect(subject.result).to be_nil + expect(subject.errors[:base].count).to be(1) + expect(subject.errors[:base].first).to include I18n.t('oauth_client.errors.oauth_returned_error') + end + end + + context 'with reply including JSON syntax error', webmock: true do + before do + stub_request(:post, File.join(host, '/apps/oauth2/api/v1/token')) + .to_return( + status: 400, + headers: { 'Content-Type' => 'application/json; charset=utf-8' }, + body: "some: very, invalid> Date: Fri, 10 Jun 2022 10:22:58 +0200 Subject: [PATCH 2/4] Address issues that were detected during review --- app/controllers/oauth_clients_controller.rb | 2 +- .../oauth_clients/connection_manager.rb | 27 ++++++++++------ lib/api/v3/utilities/path_helper.rb | 4 +++ .../oauth_clients/callback_flow_spec.rb | 32 +++++++++++++++---- 4 files changed, 48 insertions(+), 17 deletions(-) diff --git a/app/controllers/oauth_clients_controller.rb b/app/controllers/oauth_clients_controller.rb index f8f7425b0659..ba8c19c7f859 100644 --- a/app/controllers/oauth_clients_controller.rb +++ b/app/controllers/oauth_clients_controller.rb @@ -128,7 +128,7 @@ def redirect_user_or_admin(state = nil) # 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 diff --git a/app/services/oauth_clients/connection_manager.rb b/app/services/oauth_clients/connection_manager.rb index 8e1c7379842d..6e54275be04d 100644 --- a/app/services/oauth_clients/connection_manager.rb +++ b/app/services/oauth_clients/connection_manager.rb @@ -87,6 +87,10 @@ def callback_redirect_uri(state) # 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 @@ def request_new_token(options = {}) # 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 @@ def i18n_rack_oauth2_error_message(rack_oauth2_client_exception) # 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 @@ def rack_oauth_client(options = {}) 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 diff --git a/lib/api/v3/utilities/path_helper.rb b/lib/api/v3/utilities/path_helper.rb index 79176c373f7b..499ec95eac29 100644 --- a/lib/api/v3/utilities/path_helper.rb +++ b/lib/api/v3/utilities/path_helper.rb @@ -92,6 +92,10 @@ def self.root "#{root_path}api/v3" end + def self.same_origin?(url) + url.to_s.start_with? root_url + end + index :action show :action diff --git a/spec/requests/oauth_clients/callback_flow_spec.rb b/spec/requests/oauth_clients/callback_flow_spec.rb index 47053ea0b973..a7261567e60b 100644 --- a/spec/requests/oauth_clients/callback_flow_spec.rb +++ b/spec/requests/oauth_clients/callback_flow_spec.rb @@ -39,7 +39,9 @@ "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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 From cd946c71d55fdc092b873830570b6057cf53ac6a Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Fri, 10 Jun 2022 12:34:39 +0200 Subject: [PATCH 3/4] Service for reading and sanitizing the redirect URL from OAuth state param --- .../redirect_uri_from_state_service.rb | 57 +++++++++++++++ .../redirect_uri_from_state_service_spec.rb | 73 +++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 app/services/oauth_clients/redirect_uri_from_state_service.rb create mode 100644 spec/services/oauth_clients/redirect_uri_from_state_service_spec.rb diff --git a/app/services/oauth_clients/redirect_uri_from_state_service.rb b/app/services/oauth_clients/redirect_uri_from_state_service.rb new file mode 100644 index 000000000000..cb70a810aba9 --- /dev/null +++ b/app/services/oauth_clients/redirect_uri_from_state_service.rb @@ -0,0 +1,57 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "rack/oauth2" +require "uri/http" + +module OAuthClients + class RedirectUriFromStateService + def initialize(state:, cookies:) + @state = state + @cookies = cookies + end + + def call + redirect_uri = oauth_state_cookie + + if redirect_uri.present? && ::API::V3::Utilities::PathHelper::ApiV3Path::same_origin?(redirect_uri) + ServiceResult.new(success: true, result: redirect_uri) + else + ServiceResult.new(success: false) + end + end + + private + + def oauth_state_cookie + return nil if @state.blank? + + @cookies["oauth_state_#{@state}"] + end + end +end diff --git a/spec/services/oauth_clients/redirect_uri_from_state_service_spec.rb b/spec/services/oauth_clients/redirect_uri_from_state_service_spec.rb new file mode 100644 index 000000000000..7a3ef4237e90 --- /dev/null +++ b/spec/services/oauth_clients/redirect_uri_from_state_service_spec.rb @@ -0,0 +1,73 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require 'spec_helper' +require 'webmock/rspec' + +describe ::OAuthClients::RedirectUriFromStateService, type: :model do + let(:state) { 'asdf123425' } + let(:redirect_uri) { File.join(::API::V3::Utilities::PathHelper::ApiV3Path::root_url, 'foo/bar') } + let(:cookies) { { "oauth_state_#{state}": redirect_uri }.with_indifferent_access } + let(:instance) { described_class.new(state:, cookies:) } + + describe '#call' do + subject { instance.call } + + shared_examples 'failed service result' do + it 'return a failed service result' do + expect(subject).to be_failure + end + end + + context 'when cookie found' do + context 'when redirect_uri has same origin' do + it 'returns the redirect URL value from the cookie' do + expect(subject).to be_success + end + end + + context 'when redirect_uri does not share same origin' do + let(:redirect_uri) { 'https://some-other-origin.com/bla' } + + it_behaves_like 'failed service result' + end + end + + context 'when no cookie present' do + let(:cookies) { {} } + + it_behaves_like 'failed service result' + end + + context 'when no state present' do + let(:state) { nil } + + it_behaves_like 'failed service result' + end + end +end From 8a4c353381d5f166923e4e59c4fb11ce9a8dffab Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Fri, 10 Jun 2022 16:33:30 +0200 Subject: [PATCH 4/4] Protect Authorization Code flow from CSRF --- app/controllers/oauth_clients_controller.rb | 62 ++++++++++++------- .../oauth_clients/connection_manager.rb | 18 +----- .../oauth_clients/callback_flow_spec.rb | 24 +++---- 3 files changed, 54 insertions(+), 50 deletions(-) diff --git a/app/controllers/oauth_clients_controller.rb b/app/controllers/oauth_clients_controller.rb index ba8c19c7f859..77755db8948d 100644 --- a/app/controllers/oauth_clients_controller.rb +++ b/app/controllers/oauth_clients_controller.rb @@ -30,8 +30,9 @@ # "callback" endpoint. class OAuthClientsController < ApplicationController before_action :find_oauth_client - before_action :set_state + before_action :set_redirect_uri before_action :set_code + before_action :set_connection_manager # Provide the OAuth2 "callback" endpoint. # The Authorization Server redirects @@ -43,21 +44,18 @@ class OAuthClientsController < ApplicationController # state=http%3A%2F%2Flocalhost%3A4200%2Fprojects%2Fdemo-project%2Foauth2_example& # code=MQoOnUTJGFdAo5jBGD1SqnDH0PV6yioG7NoYM2zZZlK3g6LuKrGUmOxjIS1bIy7fHEfZy2WrgYcx def callback - connection_manager = OAuthClients::ConnectionManager.new(user: User.current, oauth_client: @oauth_client) - # Exchange the code with a token using a HTTP call to the Authorization Server - service_result = connection_manager.code_to_token(@code) + service_result = @connection_manager.code_to_token(@code) if service_result.success? # Redirect the user to the page that initially wanted to access the OAuth2 resource. - # "state" is a variable that encapsulates the page's URL and status. - redirect_uri = connection_manager.callback_redirect_uri(@state) - redirect_to redirect_uri + # "state" is a nonce that identifies a cookie which holds that page's URL. + redirect_to @redirect_uri else # We got a list of errors from ::OAuthClients::ConnectionManager set_oauth_errors(service_result) - redirect_user_or_admin(@state) do + redirect_user_or_admin(@redirect_uri) do # If the current user is an admin, we send her directly to the # settings that she needs to edit. redirect_to admin_settings_storage_path(@oauth_client.integration) @@ -79,34 +77,45 @@ def set_code # So this could either be an error from the Authorization Server (i.e. Nextcloud) or # ::OAuthClient::ConnectionManager has used the wrong response_type. @code = params[:code] + if @code.blank? flash[:error] = [I18n.t('oauth_client.errors.oauth_code_not_present'), I18n.t('oauth_client.errors.oauth_code_not_present_explanation')] - redirect_user_or_admin params[:state] do + redirect_user_or_admin(get_redirect_uri) do # If the current user is an admin, we send her directly to the - # settings that she needs to edit. + # settings that she needs to edit/fix. redirect_to admin_settings_storage_path(@oauth_client.integration) end end end - def set_state - # state is used by OpenProject to contain the redirection URL where to - # continue after receiving an OAuth2 access token. So it should not be blank. - @state = params[:state] - if @state.blank? + def set_redirect_uri + # redirect_uri is used by OpenProject to redirect to + # after receiving an OAuth2 access token. So it should not be blank. + service_result = ::OAuthClients::RedirectUriFromStateService + .new(state: params[:state], cookies:) + .call + + if service_result.success? + @redirect_uri = service_result.result + else + # To protect against CSRF we cancel this request. There was either no + # state parameter given, or there was no corresponding cookie present. flash[:error] = [I18n.t('oauth_client.errors.oauth_state_not_present'), I18n.t('oauth_client.errors.oauth_state_not_present_explanation')] redirect_user_or_admin(nil) do - # If the current user is an admin, we send her directly to the - # settings that she needs to edit. + # Guide the user to the settings that she needs to edit/fix. redirect_to admin_settings_storage_path(@oauth_client.integration) end end end + def set_connection_manager + @connection_manager = OAuthClients::ConnectionManager.new(user: User.current, oauth_client: @oauth_client) + end + def find_oauth_client @oauth_client = OAuthClient.find_by(client_id: params[:oauth_client_id]) if @oauth_client.nil? @@ -117,22 +126,22 @@ def find_oauth_client flash[:error] = [I18n.t('oauth_client.errors.oauth_client_not_found'), I18n.t('oauth_client.errors.oauth_client_not_found_explanation')] - redirect_user_or_admin params[:state] do + redirect_user_or_admin(get_redirect_uri) do # Something must be wrong in the storage's setup redirect_to admin_settings_storages_path end end end - def redirect_user_or_admin(state = nil) + def redirect_user_or_admin(redirect_uri = nil) # This needs to be modified as soon as we support more integration types. - if User.current.admin && state && nextcloud? + if User.current.admin && redirect_uri && nextcloud? yield - elsif ::API::V3::Utilities::PathHelper::ApiV3Path::same_origin? state + elsif redirect_uri flash[:error] = [t(:'oauth_client.errors.oauth_issue_contact_admin')] - redirect_to state + redirect_to redirect_uri else - redirect_to home_path + redirect_to ::API::V3::Utilities::PathHelper::ApiV3Path::root_url end end @@ -141,4 +150,11 @@ def nextcloud? @oauth_client.integration.is_a?(::Storages::Storage) && \ @oauth_client.integration.provider_type == 'nextcloud' end + + def get_redirect_uri + ::OAuthClients::RedirectUriFromStateService + .new(state: params[:state], cookies:) + .call + .result + end end diff --git a/app/services/oauth_clients/connection_manager.rb b/app/services/oauth_clients/connection_manager.rb index 6e54275be04d..d0bbd83afd2a 100644 --- a/app/services/oauth_clients/connection_manager.rb +++ b/app/services/oauth_clients/connection_manager.rb @@ -72,28 +72,14 @@ def refresh_token end # Redirect to the "authorize" endpoint of the OAuth2 Authorization Server. - # @param state (OAuth2 RFC) encapsulates the state of the calling page (URL + params) to return + # @param state (OAuth2 RFC) is a nonce referencing a cookie containing the calling page (URL + params) to which to + # return to at the end of the whole flow. # @param scope (OAuth2 RFC) specifies the resources to access. Nextcloud only has one global scope. def redirect_to_oauth_authorize(scope: [], state: nil) client = rack_oauth_client # Configure and start the rack-oauth2 client client.authorization_uri(scope:, state:) end - # For the OAuth2 callback page: Calculate the redirection URL that will - # point the browser at the initial page that wanted to access the OAuth2 - # protected resource. - # @param state (OAuth2 RFC) encapsulates the state of the calling page (URL + params) to return - def callback_redirect_uri(state) - # 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 - # Called by callback_page with a cryptographic "code" that indicates # that the user has successfully authorized the OAuth2 Authorization Server. # We now are going to exchange this code to a token (bearer+refresh) diff --git a/spec/requests/oauth_clients/callback_flow_spec.rb b/spec/requests/oauth_clients/callback_flow_spec.rb index a7261567e60b..a5f6d8ae62b6 100644 --- a/spec/requests/oauth_clients/callback_flow_spec.rb +++ b/spec/requests/oauth_clients/callback_flow_spec.rb @@ -39,7 +39,8 @@ "mBf4v9hNA6hXXCWHd5mZggsAa2FSOXinx9jKx1yjSoDwOPOX4k6zGEgM2radqgg1nRwXCqvIe5xZsfwqMIaTdL" + "jYnl0OpYOc6ePblzQTmnlp7RYiHW09assYEJjv9zps" end - let(:state) do + let(:state) { 'asdf1234' } + let(:redirect_uri) 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 } @@ -69,12 +70,13 @@ refresh_token: 'xyzrefreshtoken') ) allow(rack_oauth2_client).to receive(:authorization_code=) + set_cookie "oauth_state_asdf1234=#{redirect_uri}" end - shared_examples 'with errors and state param, not being admin' do - it 'redirects to URI encode in state' do + shared_examples 'with errors and state param with cookie, not being admin' do + it 'redirects to URI referenced in the state param and held in a cookie' do expect(response.status).to eq 302 - expect(response.location).to eq state + expect(response.location).to eq redirect_uri end end @@ -88,7 +90,7 @@ 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 + expect(URI(response.location).path).to eq ::API::V3::Utilities::PathHelper::ApiV3Path::root_path end end @@ -101,18 +103,18 @@ subject end - it 'redirects to the URL that was provided by the state param' do + it 'redirects to the URL that was referenced by the state param and held by a cookie' do expect(rack_oauth2_client).to have_received(:authorization_code=).with(code) expect(response.status).to eq 302 - expect(response.location).to eq state + expect(response.location).to eq redirect_uri expect(::OAuthClientToken.count).to eq 1 expect(::OAuthClientToken.last.access_token).to eq 'xyzaccesstoken' expect(::OAuthClientToken.last.refresh_token).to eq 'xyzrefreshtoken' 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" } + context 'with a OAuth state cookie containing a URL pointing to a different host' do + let(:redirect_uri) { "https://some-other-domain.com/foo/bar" } before do uri.query = URI.encode_www_form([['code', code], ['state', state]]) @@ -144,7 +146,7 @@ end context 'with current_user not being an admin' do - it_behaves_like 'with errors and state param, not being admin' + it_behaves_like 'with errors and state param with cookie, not being admin' end end end @@ -158,7 +160,7 @@ end context 'with current_user not being an admin' do - it_behaves_like 'with errors and state param, not being admin' + it_behaves_like 'with errors and state param with cookie, not being admin' end context 'with current_user being an admin' do