From 201f5cd3eec985acc25a246bf4cb277ad8ca28e0 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Thu, 12 Dec 2024 15:59:20 +0100 Subject: [PATCH] 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 ++- .../create_open_project_token.rb | 65 ++++++++++ ...=> 20241212131910_add_oidc_user_tokens.rb} | 19 ++- .../open_project/openid_connect/hooks/hook.rb | 1 + .../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 ++++--- .../create_open_project_token_spec.rb | 112 ++++++++++++++++++ spec/features/auth/consent_auth_stage_spec.rb | 9 +- 10 files changed, 244 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} (77%) create mode 100644 modules/openid_connect/app/services/openid_connect/create_open_project_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/create_open_project_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 77% 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..adba83665734 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" + + OP_AUDIENCE = "open_project".freeze + + belongs_to :session, class_name: "Sessions::UserSession", dependent: :delete + + scope :open_project, -> { where(audience: OP_AUDIENCE) } end end diff --git a/modules/openid_connect/app/services/openid_connect/create_open_project_token.rb b/modules/openid_connect/app/services/openid_connect/create_open_project_token.rb new file mode 100644 index 000000000000..60a1298ac145 --- /dev/null +++ b/modules/openid_connect/app/services/openid_connect/create_open_project_token.rb @@ -0,0 +1,65 @@ +#-- 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 CreateOpenProjectToken + def initialize(session) + @session = session + end + + def call + 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 + + user_session.oidc_user_tokens.open_project.create!(access_token:, refresh_token:) + end + + private + + def find_user_session + private_session_id = @session.id.private_id + ::Sessions::UserSession.find_by(session_id: private_session_id) + end + + def access_token + @session["omniauth.oidc_access_token"] + end + + def refresh_token + @session["omniauth.oidc_refresh_token"] + 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..919418ba3442 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.string :audience, null: false + + 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..4b39ca1aa990 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,7 @@ class Hook < OpenProject::Hook::Listener def user_logged_in(context) session = context.fetch(:session) ::OpenProject::OpenIDConnect::SessionMapper.handle_login(session) + OpenIDConnect::CreateOpenProjectToken.new(session).call 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..bb32d34798f2 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.audience).to eq "open_project" + 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/create_open_project_token_spec.rb b/modules/openid_connect/spec/services/openid_connect/create_open_project_token_spec.rb new file mode 100644 index 000000000000..ffb6a3863a48 --- /dev/null +++ b/modules/openid_connect/spec/services/openid_connect/create_open_project_token_spec.rb @@ -0,0 +1,112 @@ +#-- 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::CreateOpenProjectToken do + subject { described_class.new(session).call } + + let(:session) do + instance_double(ActionDispatch::Request::Session, + id: instance_double(Rack::Session::SessionId, private_id: 42)) + end + + let(:session_data) do + { + "omniauth.oidc_access_token" => access_token, + "omniauth.oidc_refresh_token" => refresh_token + } + end + + 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(session).to receive(:[]) { |k| session_data[k] } + 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.audience).to eq "open_project" + 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.audience).to eq "open_project" + 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 +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