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.

modules/openid_connect/db/migrate/20240806174815_add_tokens_to_oidc_user_session_links.rb → 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

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

modules/openid_connect/db/migrate/20241212104658_make_oidc_session_optional.rb → 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

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
 
       ##

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
 

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}" }

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
 

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",

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