From 12da6c6b1227bb48fea964dc967b307749242656 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Fri, 11 Oct 2024 19:21:51 +0200 Subject: [PATCH 1/4] 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 | 4 +- .../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, 252 insertions(+), 38 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..567275414f59 100644 --- a/app/services/authentication/omniauth_service.rb +++ b/app/services/authentication/omniauth_service.rb @@ -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]) From cdc304faf4bb5ecc4dd17b4e4b3efcf1df716083 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Thu, 12 Dec 2024 15:59:20 +0100 Subject: [PATCH 2/4] Change the way user tokens are stored This commit provides an alternative implementation for storing tokens compared to the parent commit. The idea is that we will not only need to store access and refresh tokens obtained via Omniauth, but also the ones to access third party services that will most likely be obtained through OAuth 2.0 Token Exchange. This structure allows to store all of these tokens in the same data model, while keeping the implementation separated from the back-channel logout logic. --- .../models/openid_connect/user_token.rb} | 15 ++- .../openid_connect/associate_user_token.rb | 61 +++++++++ ...=> 20241212131910_add_oidc_user_tokens.rb} | 19 ++- .../open_project/openid_connect/hooks/hook.rb | 5 + .../patches/sessions/user_session_patch.rb | 1 + .../openid_connect/session_mapper.rb | 7 +- .../spec/lib/session_mapper_spec.rb | 24 +--- .../spec/requests/openid_connect_spec.rb | 43 ++++--- .../associate_user_token_spec.rb | 119 ++++++++++++++++++ spec/features/auth/consent_auth_stage_spec.rb | 9 +- 10 files changed, 251 insertions(+), 52 deletions(-) rename modules/openid_connect/{db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb => app/models/openid_connect/user_token.rb} (78%) create mode 100644 modules/openid_connect/app/services/openid_connect/associate_user_token.rb rename modules/openid_connect/db/migrate/{20241212104658_make_oidc_session_optional.rb => 20241212131910_add_oidc_user_tokens.rb} (63%) create mode 100644 modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb diff --git a/modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb b/modules/openid_connect/app/models/openid_connect/user_token.rb similarity index 78% rename from modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb rename to modules/openid_connect/app/models/openid_connect/user_token.rb index 7976c0ab7dd8..ea9ee015c61c 100644 --- a/modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb +++ b/modules/openid_connect/app/models/openid_connect/user_token.rb @@ -1,6 +1,6 @@ #-- copyright # OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH +# 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. @@ -26,9 +26,14 @@ # 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 +module OpenIDConnect + class UserToken < ::ApplicationRecord + self.table_name = "oidc_user_tokens" + + IDP_AUDIENCE = "__op-idp__".freeze + + belongs_to :session, class_name: "Sessions::UserSession", dependent: :delete + + scope :idp, -> { where(audience: IDP_AUDIENCE) } end end diff --git a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb new file mode 100644 index 000000000000..60c4be3dad77 --- /dev/null +++ b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb @@ -0,0 +1,61 @@ +#-- copyright +# OpenProject is a project management system. +# 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-2017 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 AssociateUserToken + def initialize(session) + @session = session + end + + def call(access_token:, refresh_token: nil, assume_idp: false) + if access_token.blank? + Rails.logger.error("Could not associate token to session: No access token") + return + end + + user_session = find_user_session + if user_session.nil? + Rails.logger.error("Could not associate token to session: User session not found") + return + end + + token = user_session.oidc_user_tokens.build(access_token:, refresh_token:) + # We should discover further audiences from the token in the future + token.audiences << UserToken::IDP_AUDIENCE if assume_idp + + token.save! if token.audiences.any? + end + + private + + def find_user_session + private_session_id = @session.id.private_id + ::Sessions::UserSession.find_by(session_id: private_session_id) + end + end +end diff --git a/modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb b/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb similarity index 63% rename from modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb rename to modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb index bce1f9a86c65..5f2e3aecf1b6 100644 --- a/modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb +++ b/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb @@ -26,8 +26,23 @@ # See COPYRIGHT and LICENSE files for more details. #++ -class MakeOidcSessionOptional < ActiveRecord::Migration[7.1] +class AddOidcUserTokens < ActiveRecord::Migration[7.1] def change - change_column_null :oidc_user_session_links, :oidc_session, true + create_unlogged_tables = ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables + begin + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables = true + + create_table :oidc_user_tokens do |t| + t.references :session, index: true, foreign_key: { to_table: "sessions", on_delete: :cascade } + + t.string :access_token, null: false + t.string :refresh_token, null: true + t.jsonb :audiences, null: false, default: [] + + t.timestamps + end + ensure + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables = create_unlogged_tables + end end end 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 519655e2f393..b4d280e19082 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 @@ -35,6 +35,11 @@ class Hook < OpenProject::Hook::Listener def user_logged_in(context) session = context.fetch(:session) ::OpenProject::OpenIDConnect::SessionMapper.handle_login(session) + OpenIDConnect::AssociateUserToken.new(session).call( + access_token: session["omniauth.oidc_access_token"], + refresh_token: session["omniauth.oidc_refresh_token"], + assume_idp: true + ) 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 index e24bfc7544d9..9da9e2bd65ee 100644 --- 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 @@ -33,6 +33,7 @@ def self.included(base) # :nodoc: base.class_eval do has_one :oidc_session_link, class_name: "OpenIDConnect::UserSessionLink", foreign_key: "session_id" + has_many :oidc_user_tokens, class_name: "OpenIDConnect::UserToken", foreign_key: "session_id" 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 8089ea8b9879..893efde30d88 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 @@ -9,9 +9,10 @@ def self.handle_logout(logout_token) end 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"]) + oidc_session = session["omniauth.oidc_sid"] + return if oidc_session.blank? + + link = ::OpenIDConnect::UserSessionLink.new(oidc_session:) 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 0c83bf4756af..462e6b977231 100644 --- a/modules/openid_connect/spec/lib/session_mapper_spec.rb +++ b/modules/openid_connect/spec/lib/session_mapper_spec.rb @@ -35,15 +35,11 @@ let(:session_data) do { - "omniauth.oidc_sid" => oidc_session_id, - "omniauth.oidc_access_token" => access_token, - "omniauth.oidc_refresh_token" => refresh_token + "omniauth.oidc_sid" => oidc_session_id } 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] } @@ -61,25 +57,7 @@ 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 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 9c5246b3c659..56e22f1340da 100644 --- a/modules/openid_connect/spec/requests/openid_connect_spec.rb +++ b/modules/openid_connect/spec/requests/openid_connect_spec.rb @@ -72,7 +72,7 @@ let(:limit_self_registration) { false } let!(:provider) { create(:oidc_provider, slug: "keycloak", limit_self_registration:) } - it "logs in the user" do + it "signs up and logs in the user" do ## # it should redirect to the provider's openid connect authentication endpoint click_on_signin("keycloak") @@ -99,30 +99,39 @@ expect(user.active?).to be true session = Sessions::UserSession.for_user(user).first - session_link = session.oidc_session_link + 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]) - user.activate - user.save! + token = session&.oidc_user_tokens&.first + expect(token).not_to be_nil + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to eq ["__op-idp__"] + end - click_on_signin("keycloak") + context "when the user is already registered" do + before do + click_on_signin("keycloak") + redirect_from_provider("keycloak") + end - expect(response).to have_http_status :found - expect(response.location).to match /https:\/\/#{host}.*$/ + it "logs in the user" do + ## + # it should redirect to the provider again upon clicking on sign-in when the user has been activated + click_on_signin("keycloak") - ## - # it should then login the user upon the redirect back from the provider - redirect_from_provider("keycloak") + expect(response).to have_http_status :found + expect(response.location).to match /https:\/\/#{host}.*$/ - expect(response).to have_http_status :found - expect(response.location).to match /\/my\/page/ + ## + # it should then login the user upon the redirect back from the provider + redirect_from_provider("keycloak") + + expect(response).to have_http_status :found + expect(response.location).to match /\/my\/page/ + end end context "with self-registration disabled and provider respecting that", diff --git a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb new file mode 100644 index 000000000000..addf7d0d38cd --- /dev/null +++ b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb @@ -0,0 +1,119 @@ +#-- 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. +#++ +require "spec_helper" + +RSpec.describe OpenIDConnect::AssociateUserToken do + subject { described_class.new(session).call(**args) } + + let(:session) do + instance_double(ActionDispatch::Request::Session, + id: instance_double(Rack::Session::SessionId, private_id: 42)) + end + + let(:args) { { access_token:, refresh_token:, assume_idp: true } } + + let(:access_token) { "access-token-foo" } + let(:refresh_token) { "refresh-token-bar" } + + let!(:user_session) { create(:user_session, session_id: session.id.private_id) } + + before do + allow(Rails.logger).to receive(:error) + end + + it "creates a correct user token", :aggregate_failures do + expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) + + token = OpenIDConnect::UserToken.last + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to eq ["__op-idp__"] + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + + context "when there is no refresh token" do + let(:refresh_token) { nil } + + it "creates a correct user token", :aggregate_failures do + expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) + + token = OpenIDConnect::UserToken.last + expect(token.access_token).to eq access_token + expect(token.refresh_token).to be_nil + expect(token.audiences).to eq ["__op-idp__"] + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + end + + context "when there is no access token" do + let(:access_token) { nil } + + it "does not create a user token" do + expect { subject }.not_to change(OpenIDConnect::UserToken, :count) + end + + it "logs an error" do + subject + expect(Rails.logger).to have_received(:error) + end + end + + context "when the user session can't be found" do + let!(:user_session) { create(:user_session, session_id: SecureRandom.uuid) } + + it "does not create a user token" do + expect { subject }.not_to change(OpenIDConnect::UserToken, :count) + end + + it "logs an error" do + subject + expect(Rails.logger).to have_received(:error) + end + end + + context "when we are not allowed to assume the token has the IDP audience" do + let(:args) { { access_token:, refresh_token: } } + + it "does not create a user token" do + expect { subject }.not_to change(OpenIDConnect::UserToken, :count) + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + end +end diff --git a/spec/features/auth/consent_auth_stage_spec.rb b/spec/features/auth/consent_auth_stage_spec.rb index 4888e7e76993..e6463a1b376e 100644 --- a/spec/features/auth/consent_auth_stage_spec.rb +++ b/spec/features/auth/consent_auth_stage_spec.rb @@ -60,6 +60,10 @@ def expect_not_logged_in expect(page).to have_no_css(".form--field-container", text: user.login) end + before do + allow(Rails.logger).to receive(:error) + end + context "when disabled", with_settings: { consent_required: false } do it "does not show consent" do login_with user.login, user_password @@ -87,11 +91,12 @@ def expect_not_logged_in consent_required: true } do it "does not show consent" do + login_with user.login, user_password + expect(Rails.logger) - .to receive(:error) + .to have_received(:error) .at_least(:once) .with("Instance is configured to require consent, but no consent_info has been set.") - login_with user.login, user_password expect(page).to have_no_css(".account-consent") expect_logged_in end From e5f122630ee97013168647f4b92782d8c4ea8a72 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Tue, 17 Dec 2024 11:24:01 +0100 Subject: [PATCH 3/4] Store tokens on user, not session Doing so hopefully simplifies token handling a bit. It's now not required to pass specific sessions into services as long as a user is passed. This theoretically also enables us to act in the name of a user from a background job, though we have no specific plans for that yet. A possible downside is, that we now require being handed long-term tokens (i.e. tokens with offline_access scope). On the other hand, we'd have had to consider keeping our tokens fresh for the previous implementation, which we also didn't solve yet. --- app/models/sessions/user_session.rb | 2 + .../app/models/openid_connect/user_token.rb | 2 +- .../openid_connect/associate_user_token.rb | 26 +++---- .../20241212131910_add_oidc_user_tokens.rb | 19 ++---- .../lib/open_project/openid_connect/engine.rb | 2 +- .../open_project/openid_connect/hooks/hook.rb | 12 +++- .../patches/sessions/user_session_patch.rb | 1 - .../openid_connect/patches/user_patch.rb | 44 ++++++++++++ .../spec/requests/openid_connect_spec.rb | 2 +- .../associate_user_token_spec.rb | 68 +++++++++++++++---- 10 files changed, 128 insertions(+), 50 deletions(-) create mode 100644 modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb diff --git a/app/models/sessions/user_session.rb b/app/models/sessions/user_session.rb index 1a0d40e751d9..446651bd2b6c 100644 --- a/app/models/sessions/user_session.rb +++ b/app/models/sessions/user_session.rb @@ -33,6 +33,8 @@ module Sessions class UserSession < ::ApplicationRecord self.table_name = "sessions" + belongs_to :user + scope :for_user, ->(user) do user_id = user.is_a?(User) ? user.id : user.to_i diff --git a/modules/openid_connect/app/models/openid_connect/user_token.rb b/modules/openid_connect/app/models/openid_connect/user_token.rb index ea9ee015c61c..fdb5b12ea85f 100644 --- a/modules/openid_connect/app/models/openid_connect/user_token.rb +++ b/modules/openid_connect/app/models/openid_connect/user_token.rb @@ -32,7 +32,7 @@ class UserToken < ::ApplicationRecord IDP_AUDIENCE = "__op-idp__".freeze - belongs_to :session, class_name: "Sessions::UserSession", dependent: :delete + belongs_to :user scope :idp, -> { where(audience: IDP_AUDIENCE) } end diff --git a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb index 60c4be3dad77..74b3369a9472 100644 --- a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb +++ b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb @@ -28,34 +28,26 @@ module OpenIDConnect class AssociateUserToken - def initialize(session) - @session = session + def initialize(user) + @user = user end - def call(access_token:, refresh_token: nil, assume_idp: false) + def call(access_token:, refresh_token: nil, known_audiences: [], clear_previous: false) if access_token.blank? - Rails.logger.error("Could not associate token to session: No access token") + Rails.logger.error("Could not associate token to user: No access token") return end - user_session = find_user_session - if user_session.nil? - Rails.logger.error("Could not associate token to session: User session not found") + if @user.nil? + Rails.logger.error("Could not associate token to user: Can't find user") return end - token = user_session.oidc_user_tokens.build(access_token:, refresh_token:) - # We should discover further audiences from the token in the future - token.audiences << UserToken::IDP_AUDIENCE if assume_idp + @user.oidc_user_tokens.destroy_all if clear_previous + token = @user.oidc_user_tokens.build(access_token:, refresh_token:, audiences: Array(known_audiences)) + # We should discover further audiences from the token in the future token.save! if token.audiences.any? end - - private - - def find_user_session - private_session_id = @session.id.private_id - ::Sessions::UserSession.find_by(session_id: private_session_id) - end end end diff --git a/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb b/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb index 5f2e3aecf1b6..d8bab0c5cf97 100644 --- a/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb +++ b/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb @@ -28,21 +28,14 @@ class AddOidcUserTokens < ActiveRecord::Migration[7.1] def change - create_unlogged_tables = ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables - begin - ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables = true + create_table :oidc_user_tokens do |t| + t.references :user, null: false, index: true, foreign_key: { on_delete: :cascade } - create_table :oidc_user_tokens do |t| - t.references :session, index: true, foreign_key: { to_table: "sessions", on_delete: :cascade } + t.string :access_token, null: false + t.string :refresh_token, null: true + t.jsonb :audiences, null: false, default: [] - t.string :access_token, null: false - t.string :refresh_token, null: true - t.jsonb :audiences, null: false, default: [] - - t.timestamps - end - ensure - ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.create_unlogged_tables = create_unlogged_tables + t.timestamps end 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 ced2a8447617..145d66bfcf5d 100644 --- a/modules/openid_connect/lib/open_project/openid_connect/engine.rb +++ b/modules/openid_connect/lib/open_project/openid_connect/engine.rb @@ -25,7 +25,7 @@ class Engine < ::Rails::Engine openid_connect/auth_provider-custom.png ) - patches %i[Sessions::UserSession] + patches %i[Sessions::UserSession User] class_inflection_override("openid_connect" => "OpenIDConnect") 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 b4d280e19082..a3bf89f7111f 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 @@ -35,11 +35,19 @@ class Hook < OpenProject::Hook::Listener def user_logged_in(context) session = context.fetch(:session) ::OpenProject::OpenIDConnect::SessionMapper.handle_login(session) - OpenIDConnect::AssociateUserToken.new(session).call( + + user = context.fetch(:user) + + # We clear previous tokens while adding this one to avoid keeping + # stale tokens around (and to avoid piling up duplicate IDP tokens) + # -> Fresh login causes fresh set of tokens + OpenIDConnect::AssociateUserToken.new(user).call( access_token: session["omniauth.oidc_access_token"], refresh_token: session["omniauth.oidc_refresh_token"], - assume_idp: true + known_audiences: [OpenIDConnect::UserToken::IDP_AUDIENCE], + clear_previous: true ) + 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 index 9da9e2bd65ee..e24bfc7544d9 100644 --- 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 @@ -33,7 +33,6 @@ def self.included(base) # :nodoc: base.class_eval do has_one :oidc_session_link, class_name: "OpenIDConnect::UserSessionLink", foreign_key: "session_id" - has_many :oidc_user_tokens, class_name: "OpenIDConnect::UserToken", foreign_key: "session_id" end end diff --git a/modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb b/modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb new file mode 100644 index 000000000000..f7269eaab014 --- /dev/null +++ b/modules/openid_connect/lib/open_project/openid_connect/patches/user_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::UserPatch + def self.included(base) # :nodoc: + base.extend(ClassMethods) + base.include(InstanceMethods) + + base.class_eval do + has_many :oidc_user_tokens, class_name: "OpenIDConnect::UserToken", foreign_key: "user_id" + end + end + + module ClassMethods + end + + module InstanceMethods + 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 56e22f1340da..da0dc9a50b40 100644 --- a/modules/openid_connect/spec/requests/openid_connect_spec.rb +++ b/modules/openid_connect/spec/requests/openid_connect_spec.rb @@ -104,7 +104,7 @@ expect(session_link).not_to be_nil expect(session_link.oidc_session).to eq oidc_sid - token = session&.oidc_user_tokens&.first + token = user.oidc_user_tokens.first expect(token).not_to be_nil expect(token.access_token).to eq access_token expect(token.refresh_token).to eq refresh_token diff --git a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb index addf7d0d38cd..98743ef0d014 100644 --- a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb +++ b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb @@ -28,20 +28,15 @@ require "spec_helper" RSpec.describe OpenIDConnect::AssociateUserToken do - subject { described_class.new(session).call(**args) } + subject { described_class.new(user).call(**args) } - let(:session) do - instance_double(ActionDispatch::Request::Session, - id: instance_double(Rack::Session::SessionId, private_id: 42)) - end + let(:user) { create(:user) } - let(:args) { { access_token:, refresh_token:, assume_idp: true } } + let(:args) { { access_token:, refresh_token:, known_audiences: ["io"] } } let(:access_token) { "access-token-foo" } let(:refresh_token) { "refresh-token-bar" } - let!(:user_session) { create(:user_session, session_id: session.id.private_id) } - before do allow(Rails.logger).to receive(:error) end @@ -50,9 +45,10 @@ expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) token = OpenIDConnect::UserToken.last + expect(token.user_id).to eq user.id expect(token.access_token).to eq access_token expect(token.refresh_token).to eq refresh_token - expect(token.audiences).to eq ["__op-idp__"] + expect(token.audiences).to eq ["io"] end it "logs no error" do @@ -67,9 +63,10 @@ expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) token = OpenIDConnect::UserToken.last + expect(token.user_id).to eq user.id expect(token.access_token).to eq access_token expect(token.refresh_token).to be_nil - expect(token.audiences).to eq ["__op-idp__"] + expect(token.audiences).to eq ["io"] end it "logs no error" do @@ -91,8 +88,8 @@ end end - context "when the user session can't be found" do - let!(:user_session) { create(:user_session, session_id: SecureRandom.uuid) } + context "when no user was passed" do + let(:user) { nil } it "does not create a user token" do expect { subject }.not_to change(OpenIDConnect::UserToken, :count) @@ -104,8 +101,8 @@ end end - context "when we are not allowed to assume the token has the IDP audience" do - let(:args) { { access_token:, refresh_token: } } + context "when there is no audience" do + let(:args) { { access_token:, refresh_token:, known_audiences: [] } } it "does not create a user token" do expect { subject }.not_to change(OpenIDConnect::UserToken, :count) @@ -116,4 +113,47 @@ expect(Rails.logger).not_to have_received(:error) end end + + context "when another user token existed before" do + let!(:existing_token) { user.oidc_user_tokens.create!(access_token: "existing", audiences: ["blubb"]) } + + it "keeps the existing token" do + subject + expect(OpenIDConnect::UserToken.find_by(id: existing_token.id)).to be_present + end + + it "creates a correct user token", :aggregate_failures do + expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) + + token = OpenIDConnect::UserToken.last + expect(token.user_id).to eq user.id + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to eq ["io"] + end + + context "and when previous tokens shall be cleared" do + let(:args) { { access_token:, refresh_token:, known_audiences: ["io"], clear_previous: true } } + + it "deletes the previous token" do + subject + expect(OpenIDConnect::UserToken.find_by(id: existing_token.id)).to be_nil + end + + it "creates a correct user token", :aggregate_failures do + expect { subject }.not_to change(OpenIDConnect::UserToken, :count) + + token = OpenIDConnect::UserToken.last + expect(token.user_id).to eq user.id + expect(token.access_token).to eq access_token + expect(token.refresh_token).to eq refresh_token + expect(token.audiences).to eq ["io"] + end + + it "logs no error" do + subject + expect(Rails.logger).not_to have_received(:error) + end + end + end end From 543f8decf342b6c6a0a823c209c28d472f87821d Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 13 Jan 2025 16:19:19 +0100 Subject: [PATCH 4/4] Add some frozen string literal comments Those places were noticed by Rubocop after rebasing a feature branch onto the dev branch. --- app/models/sessions/user_session.rb | 2 ++ .../app/models/openid_connect/user_session_link.rb | 2 ++ .../openid_connect/app/models/openid_connect/user_token.rb | 4 +++- .../app/services/openid_connect/associate_user_token.rb | 2 ++ .../db/migrate/20241212131910_add_oidc_user_tokens.rb | 2 ++ .../openid_connect/lib/open_project/openid_connect/patches.rb | 2 ++ .../openid_connect/patches/sessions/user_session_patch.rb | 2 ++ .../lib/open_project/openid_connect/patches/user_patch.rb | 2 ++ .../spec/services/openid_connect/associate_user_token_spec.rb | 2 ++ 9 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/models/sessions/user_session.rb b/app/models/sessions/user_session.rb index 446651bd2b6c..0f81c8d30293 100644 --- a/app/models/sessions/user_session.rb +++ b/app/models/sessions/user_session.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH 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 3315ca939638..e15000a288ac 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,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) 2012-2024 the OpenProject GmbH diff --git a/modules/openid_connect/app/models/openid_connect/user_token.rb b/modules/openid_connect/app/models/openid_connect/user_token.rb index fdb5b12ea85f..485d14b971a3 100644 --- a/modules/openid_connect/app/models/openid_connect/user_token.rb +++ b/modules/openid_connect/app/models/openid_connect/user_token.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) 2012-2024 the OpenProject GmbH @@ -30,7 +32,7 @@ module OpenIDConnect class UserToken < ::ApplicationRecord self.table_name = "oidc_user_tokens" - IDP_AUDIENCE = "__op-idp__".freeze + IDP_AUDIENCE = "__op-idp__" belongs_to :user diff --git a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb index 74b3369a9472..893107e86478 100644 --- a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb +++ b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is a project management system. # Copyright (C) the OpenProject GmbH diff --git a/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb b/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb index d8bab0c5cf97..fea407b84f0a 100644 --- a/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb +++ b/modules/openid_connect/db/migrate/20241212131910_add_oidc_user_tokens.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH diff --git a/modules/openid_connect/lib/open_project/openid_connect/patches.rb b/modules/openid_connect/lib/open_project/openid_connect/patches.rb index ff3c89768f2e..46d0ade680f3 100644 --- a/modules/openid_connect/lib/open_project/openid_connect/patches.rb +++ b/modules/openid_connect/lib/open_project/openid_connect/patches.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH 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 index e24bfc7544d9..aa960866aa9d 100644 --- 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 @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH diff --git a/modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb b/modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb index f7269eaab014..6a0ce7669482 100644 --- a/modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb +++ b/modules/openid_connect/lib/open_project/openid_connect/patches/user_patch.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH diff --git a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb index 98743ef0d014..0d80085ff57d 100644 --- a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb +++ b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH