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

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

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

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

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

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

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

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
 

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

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

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