From 567b1d7f33a6d236456486725b32b339e6cd6ed4 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Fri, 11 Oct 2024 19:21:51 +0200 Subject: [PATCH] Save OIDC tokens to OpenProject database. Storing tokens in the database to have them available for requests to third parties (e.g. Nextcloud) later. The OIDC session is now marked as optional, since the session link is also used to store access and refresh tokens associated with the session. Those tokens might be present, even if the session id (which belongs to the optional OIDC Back-Channel Logout specification) is missing. --- .../authentication/omniauth_service.rb | 6 +-- .../openid_connect/user_session_link.rb | 28 ++++++++++ ...5_add_tokens_to_oidc_user_session_links.rb | 34 +++++++++++++ ...241212104658_make_oidc_session_optional.rb | 33 ++++++++++++ .../lib/open_project/openid_connect/engine.rb | 8 ++- .../open_project/openid_connect/hooks/hook.rb | 18 ++++--- .../open_project/openid_connect/patches.rb | 30 +++++++++++ .../patches/sessions/user_session_patch.rb | 44 ++++++++++++++++ .../openid_connect/session_mapper.rb | 11 ++-- .../spec/lib/session_mapper_spec.rb | 51 ++++++++++++++----- .../spec/requests/openid_connect_spec.rb | 29 +++++++---- 11 files changed, 253 insertions(+), 39 deletions(-) create mode 100644 modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb create mode 100644 modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb create mode 100644 modules/openid_connect/lib/open_project/openid_connect/patches.rb create mode 100644 modules/openid_connect/lib/open_project/openid_connect/patches/sessions/user_session_patch.rb diff --git a/app/services/authentication/omniauth_service.rb b/app/services/authentication/omniauth_service.rb index 47f18d600691..a5e2365dea97 100644 --- a/app/services/authentication/omniauth_service.rb +++ b/app/services/authentication/omniauth_service.rb @@ -62,7 +62,7 @@ def call(additional_user_params = nil) # Create or update the user from omniauth # and assign non-nil parameters from the registration form - if any - assignable_params = (additional_user_params || {}).reject { |_, v| v.nil? } + assignable_params = (additional_user_params || {}).compact update_user_from_omniauth!(assignable_params) # If we have a new or invited user, we still need to register them @@ -165,7 +165,7 @@ def find_existing_user def remap_existing_user return unless Setting.oauth_allow_remapping_of_existing_users? - User.not_builtin.find_by_login(user_attributes[:login]) # rubocop:disable Rails/DynamicFindBy + User.not_builtin.find_by_login(user_attributes[:login]) end ## @@ -285,7 +285,7 @@ def identity_url_from_omniauth # Try to provide some context of the auth_hash in case of errors def auth_uid hash = auth_hash || {} - hash.dig(:info, :uid) || hash.dig(:uid) || "unknown" + hash.dig(:info, :uid) || hash[:uid] || "unknown" end end end diff --git a/modules/openid_connect/app/models/openid_connect/user_session_link.rb b/modules/openid_connect/app/models/openid_connect/user_session_link.rb index baa3caab442a..3315ca939638 100644 --- a/modules/openid_connect/app/models/openid_connect/user_session_link.rb +++ b/modules/openid_connect/app/models/openid_connect/user_session_link.rb @@ -1,3 +1,31 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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. +#++ + module OpenIDConnect class UserSessionLink < ::ApplicationRecord self.table_name = "oidc_user_session_links" diff --git a/modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb b/modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb new file mode 100644 index 000000000000..7976c0ab7dd8 --- /dev/null +++ b/modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb @@ -0,0 +1,34 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +class AddTokensToOidcUserSessionLinks < ActiveRecord::Migration[7.1] + def change + add_column :oidc_user_session_links, :access_token, :string + add_column :oidc_user_session_links, :refresh_token, :string + end +end diff --git a/modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb b/modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb new file mode 100644 index 000000000000..bce1f9a86c65 --- /dev/null +++ b/modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb @@ -0,0 +1,33 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +class MakeOidcSessionOptional < ActiveRecord::Migration[7.1] + def change + change_column_null :oidc_user_session_links, :oidc_session, true + end +end diff --git a/modules/openid_connect/lib/open_project/openid_connect/engine.rb b/modules/openid_connect/lib/open_project/openid_connect/engine.rb index bde77b42ab7f..ced2a8447617 100644 --- a/modules/openid_connect/lib/open_project/openid_connect/engine.rb +++ b/modules/openid_connect/lib/open_project/openid_connect/engine.rb @@ -25,6 +25,8 @@ class Engine < ::Rails::Engine openid_connect/auth_provider-custom.png ) + patches %i[Sessions::UserSession] + class_inflection_override("openid_connect" => "OpenIDConnect") register_auth_providers do @@ -49,7 +51,11 @@ class Engine < ::Rails::Engine end # Remember oidc session values when logging in user - h[:retain_from_session] = %w[omniauth.oidc_sid] + h[:retain_from_session] = %w[ + omniauth.oidc_sid + omniauth.oidc_access_token + omniauth.oidc_refresh_token + ] h[:backchannel_logout_callback] = ->(logout_token) do ::OpenProject::OpenIDConnect::SessionMapper.handle_logout(logout_token) diff --git a/modules/openid_connect/lib/open_project/openid_connect/hooks/hook.rb b/modules/openid_connect/lib/open_project/openid_connect/hooks/hook.rb index 20894a869de6..519655e2f393 100644 --- a/modules/openid_connect/lib/open_project/openid_connect/hooks/hook.rb +++ b/modules/openid_connect/lib/open_project/openid_connect/hooks/hook.rb @@ -33,17 +33,21 @@ class Hook < OpenProject::Hook::Listener # Once the user has signed in and has an oidc session # we want to map that to the internal session def user_logged_in(context) - session = context[:session] - oidc_sid = session["omniauth.oidc_sid"] - return if oidc_sid.nil? - - ::OpenProject::OpenIDConnect::SessionMapper.handle_login(oidc_sid, session) + session = context.fetch(:session) + ::OpenProject::OpenIDConnect::SessionMapper.handle_login(session) end ## # Called once omniauth has returned with an auth hash - # NOTE: It's a passthrough as we no longer persist the access token into the cookie - def omniauth_user_authorized(_context); end + def omniauth_user_authorized(context) + controller = context.fetch(:controller) + session = controller.session + + session["omniauth.oidc_access_token"] = context.dig(:auth_hash, :credentials, :token) + session["omniauth.oidc_refresh_token"] = context.dig(:auth_hash, :credentials, :refresh_token) + + nil + end end end end diff --git a/modules/openid_connect/lib/open_project/openid_connect/patches.rb b/modules/openid_connect/lib/open_project/openid_connect/patches.rb new file mode 100644 index 000000000000..ff3c89768f2e --- /dev/null +++ b/modules/openid_connect/lib/open_project/openid_connect/patches.rb @@ -0,0 +1,30 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +module OpenProject::OpenIDConnect::Patches +end diff --git a/modules/openid_connect/lib/open_project/openid_connect/patches/sessions/user_session_patch.rb b/modules/openid_connect/lib/open_project/openid_connect/patches/sessions/user_session_patch.rb new file mode 100644 index 000000000000..e24bfc7544d9 --- /dev/null +++ b/modules/openid_connect/lib/open_project/openid_connect/patches/sessions/user_session_patch.rb @@ -0,0 +1,44 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +module OpenProject::OpenIDConnect::Patches::Sessions::UserSessionPatch + def self.included(base) # :nodoc: + base.extend(ClassMethods) + base.include(InstanceMethods) + + base.class_eval do + has_one :oidc_session_link, class_name: "OpenIDConnect::UserSessionLink", foreign_key: "session_id" + end + end + + module ClassMethods + end + + module InstanceMethods + end +end diff --git a/modules/openid_connect/lib/open_project/openid_connect/session_mapper.rb b/modules/openid_connect/lib/open_project/openid_connect/session_mapper.rb index 13107bcffd60..8089ea8b9879 100644 --- a/modules/openid_connect/lib/open_project/openid_connect/session_mapper.rb +++ b/modules/openid_connect/lib/open_project/openid_connect/session_mapper.rb @@ -8,13 +8,10 @@ def self.handle_logout(logout_token) raise e end - def self.handle_login(oidc_session, session) - if oidc_session.blank? - Rails.logger.info { "No OIDC session returned from provider. Cannot map session for later logouts." } - return - end - - link = ::OpenIDConnect::UserSessionLink.new(oidc_session:) + def self.handle_login(session) + link = ::OpenIDConnect::UserSessionLink.new(oidc_session: session["omniauth.oidc_sid"], + access_token: session["omniauth.oidc_access_token"], + refresh_token: session["omniauth.oidc_refresh_token"]) new(link).link_to_internal!(session) rescue StandardError => e Rails.logger.error { "Failed to map OIDC session to internal: #{e.message}" } diff --git a/modules/openid_connect/spec/lib/session_mapper_spec.rb b/modules/openid_connect/spec/lib/session_mapper_spec.rb index 96bab9c1d940..0c83bf4756af 100644 --- a/modules/openid_connect/spec/lib/session_mapper_spec.rb +++ b/modules/openid_connect/spec/lib/session_mapper_spec.rb @@ -28,23 +28,32 @@ require "spec_helper" RSpec.describe OpenProject::OpenIDConnect::SessionMapper do - let(:mock_session) do - Class.new(Rack::Session::Abstract::SessionHash) do - def initialize(id) - super(nil, nil) - @id = Rack::Session::SessionId.new(id) - @data = {} - @loaded = true - end - end + let(:session) do + instance_double(ActionDispatch::Request::Session, + id: instance_double(Rack::Session::SessionId, private_id: 42)) + end + + let(:session_data) do + { + "omniauth.oidc_sid" => oidc_session_id, + "omniauth.oidc_access_token" => access_token, + "omniauth.oidc_refresh_token" => refresh_token + } + end + + let(:oidc_session_id) { "oidc_sid_foo" } + let(:access_token) { "access_token_bar" } + let(:refresh_token) { "refresh_token_baz" } + + before do + allow(session).to receive(:[]) { |k| session_data[k] } end describe "handle_login" do - let(:session) { mock_session.new("foo") } let!(:plain_session) { create(:user_session, session_id: session.id.private_id) } let!(:user_session) { Sessions::UserSession.find_by(session_id: plain_session.session_id) } - subject { described_class.handle_login "oidc_sid_foo", session } + subject { described_class.handle_login session } it "creates a user link object" do expect { subject }.to change(OpenIDConnect::UserSessionLink, :count).by(1) @@ -52,7 +61,25 @@ def initialize(id) expect(link).to be_present expect(link.session).to eq user_session - expect(link.oidc_session).to eq "oidc_sid_foo" + expect(link.access_token).to eq access_token + expect(link.oidc_session).to eq oidc_session_id + expect(link.refresh_token).to eq refresh_token + end + + context "when there is only an access token" do + let(:oidc_session_id) { nil } + let(:refresh_token) { nil } + + it "creates a user link object" do + expect { subject }.to change(OpenIDConnect::UserSessionLink, :count).by(1) + link = OpenIDConnect::UserSessionLink.find_by(session_id: user_session.id) + + expect(link).to be_present + expect(link.session).to eq user_session + expect(link.access_token).to eq access_token + expect(link.oidc_session).to be_nil + expect(link.refresh_token).to be_nil + end end end diff --git a/modules/openid_connect/spec/requests/openid_connect_spec.rb b/modules/openid_connect/spec/requests/openid_connect_spec.rb index 88a9fc4bdd00..9c5246b3c659 100644 --- a/modules/openid_connect/spec/requests/openid_connect_spec.rb +++ b/modules/openid_connect/spec/requests/openid_connect_spec.rb @@ -46,23 +46,26 @@ family_name: "Wurst" } end + let(:access_token) { "foo-bar-baz" } + let(:refresh_token) { "refreshing-foo-bar-baz" } + let(:oidc_sid) { "oidc-session-id-42" } before do # The redirect will include an authorisation code. # Since we don't actually get a valid code in the test we will stub the resulting AccessToken. allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) do - OpenIDConnect::AccessToken.new client: self, access_token: "foo bar baz" + instance_double(OpenIDConnect::AccessToken, + access_token:, + refresh_token:, + userinfo!: OpenIDConnect::ResponseObject::UserInfo.new(user_info), + id_token: "not-nil").as_null_object end - # Using the granted AccessToken the client then performs another request to the OpenID Connect - # provider to retrieve user information such as name and email address. - # Since the test is not supposed to make an actual call it is be stubbed too. - allow_any_instance_of(OpenIDConnect::AccessToken).to receive(:userinfo!).and_return( - OpenIDConnect::ResponseObject::UserInfo.new(user_info) + # We are also stubbing the way that an ID token would be decoded, so that the omniauth-openid-connect + # strategy can fill the session id as well + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode).and_return( + instance_double(OpenIDConnect::ResponseObject::IdToken, sid: oidc_sid).as_null_object ) - - # Enable storing the access token in a cookie is not necessary since it is currently hard wired to always - # be true. end describe "sign-up and login" do @@ -95,6 +98,14 @@ expect(user).not_to be_nil expect(user.active?).to be true + session = Sessions::UserSession.for_user(user).first + session_link = session.oidc_session_link + + expect(session_link).not_to be_nil + expect(session_link.oidc_session).to eq oidc_sid + expect(session_link.access_token).to eq access_token + expect(session_link.refresh_token).to eq refresh_token + ## # it should redirect to the provider again upon clicking on sign-in when the user has been activated user = User.find_by(mail: user_info[:email])