From 26f4ebc8f924406a2702f1a99dcccf85e17eab4a Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 9 Feb 2024 16:55:56 +1100
Subject: [PATCH 01/13] Remove unnecessary test code

---
 spec/requests/omniauth_callbacks_controller_spec.rb | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index b96e79e7b0..f6ac2aa725 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -10,9 +10,6 @@ describe OmniauthCallbacksController, type: :request do
   subject do
     login_as user
     post '/user/spree_user/auth/openid_connect/callback', params: { code: 'code123' }
-
-    request.env['devise.mapping'] = Devise.mappings[:spree_user]
-    request.env['omniauth.auth'] = omniauth_response
   end
 
   let(:user) { create(:user) }
@@ -35,7 +32,6 @@ describe OmniauthCallbacksController, type: :request do
 
       expect(user.provider).to eq "openid_connect"
       expect(user.uid).to eq "john@doe.com"
-      expect(request.cookies[:omniauth_connect]).to be_nil
       expect(response.status).to eq(302)
     end
   end
@@ -50,7 +46,6 @@ describe OmniauthCallbacksController, type: :request do
 
       expect(user.provider).to be_nil
       expect(user.uid).to be_nil
-      expect(request.cookies[:omniauth_connect]).to be_nil
       expect(response.status).to eq(302)
     end
   end

From 60dc7107604df9d727ad8b3678dce2fda2edd9dc Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 9 Feb 2024 17:08:22 +1100
Subject: [PATCH 02/13] Refactor OIDC callback spec

* Clarify that it's a request spec, not testing a controller directly.
* Use `before` block to avoid side effects changing config at load time.
* Better name the test action as request instead of plain "subject".
* Move assignments into `before` block instead of variable.
---
 .../omniauth_callbacks_controller_spec.rb     | 22 +++++++++++--------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index f6ac2aa725..8c42f305c6 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -2,20 +2,24 @@
 
 require 'spec_helper'
 
-describe OmniauthCallbacksController, type: :request do
+# Devise calls OmniauthCallbacksController for OpenID Connect callbacks.
+describe '/user/spree_user/auth/openid_connect/callback', type: :request do
   include AuthenticationHelper
 
-  OmniAuth.config.test_mode = true
+  let(:user) { create(:user) }
+  let(:params) { { code: 'code123' } }
 
-  subject do
+  before do
+    OmniAuth.config.test_mode = true
     login_as user
-    post '/user/spree_user/auth/openid_connect/callback', params: { code: 'code123' }
   end
 
-  let(:user) { create(:user) }
+  def request!
+    post(self.class.top_level_description, params:)
+  end
 
   context 'when the omniauth setup is returning with an authorization' do
-    let!(:omniauth_response) do
+    before do
       OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new(
         'provider' => 'openid_connect',
         'uid' => 'john@doe.com',
@@ -28,7 +32,7 @@ describe OmniauthCallbacksController, type: :request do
     end
 
     it 'is successful' do
-      subject
+      request!
 
       expect(user.provider).to eq "openid_connect"
       expect(user.uid).to eq "john@doe.com"
@@ -37,12 +41,12 @@ describe OmniauthCallbacksController, type: :request do
   end
 
   context 'when the omniauth openid_connect is mocked with an error' do
-    let!(:omniauth_response) do
+    before do
       OmniAuth.config.mock_auth[:openid_connect] = :invalid_credentials
     end
 
     it 'fails with bad auth data' do
-      subject
+      request!
 
       expect(user.provider).to be_nil
       expect(user.uid).to be_nil

From 4d680e5fd10938d6b260b62ce1ef2b52685be127 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 9 Feb 2024 17:13:27 +1100
Subject: [PATCH 03/13] Use recorded auth hash including all tokens

We want to store the access and refresh token as well.
---
 config/initializers/devise.rb                 | 10 +++-
 spec/fixtures/files/omniauth.auth.json        | 48 +++++++++++++++++++
 .../omniauth_callbacks_controller_spec.rb     | 12 ++---
 3 files changed, 60 insertions(+), 10 deletions(-)
 create mode 100644 spec/fixtures/files/omniauth.auth.json

diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index ab762473db..5c3c0ca585 100644
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -145,7 +145,13 @@ end
 
 if ENV["OPENID_APP_ID"].present? && ENV["OPENID_APP_SECRET"].present?
   Devise.setup do |config|
-    protocol = Rails.env.development? ? "http://" : "https://"
+    site = if Rails.env.development?
+             # The lescommuns server accepts localhost:3000 as valid.
+             # So you can test in development.
+             "http://localhost:3000"
+           else
+             "https://#{ENV["SITE_URL"]}"
+           end
     config.omniauth :openid_connect, {
       name: :openid_connect,
       issuer: "https://login.lescommuns.org/auth/realms/data-food-consortium",
@@ -158,7 +164,7 @@ if ENV["OPENID_APP_ID"].present? && ENV["OPENID_APP_SECRET"].present?
       client_options: {
         identifier: ENV["OPENID_APP_ID"],
         secret: ENV["OPENID_APP_SECRET"],
-        redirect_uri: "#{protocol}#{ENV["SITE_URL"]}/user/spree_user/auth/openid_connect/callback",
+        redirect_uri: "#{site}/user/spree_user/auth/openid_connect/callback",
         jwks_uri: 'https://login.lescommuns.org/auth/realms/data-food-consortium/protocol/openid-connect/certs'
       }
     }
diff --git a/spec/fixtures/files/omniauth.auth.json b/spec/fixtures/files/omniauth.auth.json
new file mode 100644
index 0000000000..d89f690f04
--- /dev/null
+++ b/spec/fixtures/files/omniauth.auth.json
@@ -0,0 +1,48 @@
+{
+  "provider": "openid_connect",
+  "uid": "ofn@example.com",
+  "info": {
+    "name": "OFN Developer",
+    "email": "ofn@example.com",
+    "email_verified": false,
+    "nickname": "ofn@example.com",
+    "first_name": "OFN",
+    "last_name": "Developer",
+    "gender": null,
+    "image": null,
+    "phone": null,
+    "urls": {
+      "website": null
+    }
+  },
+  "credentials": {
+    "id_token": "ey...id_token...zg",
+    "token": "ey...token...9g",
+    "refresh_token": "ey...refresh_token...bk",
+    "expires_in": 1800,
+    "scope": "openid profile email"
+  },
+  "extra": {
+    "raw_info": {
+      "sub": "97da8027-a7a9-44c8-9cfd-ad639cec8630",
+      "email_verified": false,
+      "name": "OFN Developer",
+      "preferred_username": "ofn@example.com",
+      "given_name": "OFN",
+      "family_name": "Developer",
+      "email": "ofn@example.com",
+      "exp": 1707458565,
+      "iat": 1707456765,
+      "auth_time": 1707456763,
+      "jti": "00643994-5914-4699-96b0-2b4a308fca65",
+      "iss": "https://login.lescommuns.org/auth/realms/data-food-consortium",
+      "aud": "coopcircuits",
+      "typ": "ID",
+      "azp": "coopcircuits",
+      "nonce": "215831991b35c70d43fb2102ee78be55",
+      "session_state": "8b5725a1-e83a-4f78-a54b-36c5a2983dd4",
+      "at_hash": "RT8oVVJdFDiaytyDxHJLyQ",
+      "sid": "8b5725a1-e83a-4f78-a54b-36c5a2983dd4"
+    }
+  }
+}
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index 8c42f305c6..6c2034bfe7 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -19,15 +19,11 @@ describe '/user/spree_user/auth/openid_connect/callback', type: :request do
   end
 
   context 'when the omniauth setup is returning with an authorization' do
+    # The auth hash data has been observed by connecting to the Keycloak server
+    # https://login.lescommuns.org/.
     before do
       OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new(
-        'provider' => 'openid_connect',
-        'uid' => 'john@doe.com',
-        'info' => {
-          'email' => 'john@doe.com',
-          'first_name' => 'John',
-          'last_name' => 'Doe'
-        }
+        JSON.parse(file_fixture("omniauth.auth.json").read)
       )
     end
 
@@ -35,7 +31,7 @@ describe '/user/spree_user/auth/openid_connect/callback', type: :request do
       request!
 
       expect(user.provider).to eq "openid_connect"
-      expect(user.uid).to eq "john@doe.com"
+      expect(user.uid).to eq "ofn@example.com"
       expect(response.status).to eq(302)
     end
   end

From b4ee24368ce127de8f8a153acfaec264d653982f Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 13 Feb 2024 15:39:28 +1100
Subject: [PATCH 04/13] Add model for OIDC accounts

The provider name and uid are currently stored on the user model but
it's better to move them to their own table. They are only needed in
certain situations, only some users have an account and we are now
storing a lot more.
---
 app/models/oidc_account.rb                     |  9 +++++++++
 .../20240213042618_create_oidc_accounts.rb     | 18 ++++++++++++++++++
 db/schema.rb                                   | 15 ++++++++++++++-
 spec/models/oidc_account_spec.rb               | 18 ++++++++++++++++++
 4 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 app/models/oidc_account.rb
 create mode 100644 db/migrate/20240213042618_create_oidc_accounts.rb
 create mode 100644 spec/models/oidc_account_spec.rb

diff --git a/app/models/oidc_account.rb b/app/models/oidc_account.rb
new file mode 100644
index 0000000000..9d1eb8b752
--- /dev/null
+++ b/app/models/oidc_account.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class OidcAccount < ApplicationRecord
+  belongs_to :user, class_name: "Spree::User"
+
+  # When a user authenticates via token, the `uid` should be mapped to only one
+  # OFN user and therefore it needs to be unique.
+  validates :uid, presence: true, uniqueness: true
+end
diff --git a/db/migrate/20240213042618_create_oidc_accounts.rb b/db/migrate/20240213042618_create_oidc_accounts.rb
new file mode 100644
index 0000000000..7a77e8b716
--- /dev/null
+++ b/db/migrate/20240213042618_create_oidc_accounts.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class CreateOidcAccounts < ActiveRecord::Migration[7.0]
+  def change
+    create_table :oidc_accounts do |t|
+      # We may allow multiple OIDC accounts per user in the future but for now
+      # we assume only one and therefore make this unique.
+      t.belongs_to :user, null: false, foreign_key: { to_table: :spree_users },
+                          index: { unique: true }
+      t.string :provider
+      t.string :uid, null: false, index: { unique: true }
+      t.string :token
+      t.string :refresh_token
+
+      t.timestamps
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 312cf4e23e..0aad1ebf69 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_01_15_022359) do
+ActiveRecord::Schema[7.0].define(version: 2024_02_13_042618) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_stat_statements"
   enable_extension "plpgsql"
@@ -308,6 +308,18 @@ ActiveRecord::Schema[7.0].define(version: 2024_01_15_022359) do
     t.index ["order_id"], name: "index_invoices_on_order_id"
   end
 
+  create_table "oidc_accounts", force: :cascade do |t|
+    t.bigint "user_id", null: false
+    t.string "provider"
+    t.string "uid", null: false
+    t.string "token"
+    t.string "refresh_token"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["uid"], name: "index_oidc_accounts_on_uid", unique: true
+    t.index ["user_id"], name: "index_oidc_accounts_on_user_id", unique: true
+  end
+
   create_table "order_cycle_schedules", id: :serial, force: :cascade do |t|
     t.integer "order_cycle_id", null: false
     t.integer "schedule_id", null: false
@@ -1146,6 +1158,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_01_15_022359) do
   add_foreign_key "inventory_items", "enterprises"
   add_foreign_key "inventory_items", "spree_variants", column: "variant_id"
   add_foreign_key "invoices", "spree_orders", column: "order_id"
+  add_foreign_key "oidc_accounts", "spree_users", column: "user_id"
   add_foreign_key "order_cycle_schedules", "order_cycles", name: "oc_schedules_order_cycle_id_fk"
   add_foreign_key "order_cycle_schedules", "schedules", name: "oc_schedules_schedule_id_fk"
   add_foreign_key "order_cycles", "enterprises", column: "coordinator_id", name: "order_cycles_coordinator_id_fk"
diff --git a/spec/models/oidc_account_spec.rb b/spec/models/oidc_account_spec.rb
new file mode 100644
index 0000000000..4e70d550b2
--- /dev/null
+++ b/spec/models/oidc_account_spec.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe OidcAccount, type: :model do
+  describe "associations and validations" do
+    subject {
+      OidcAccount.new(
+        user: build(:user),
+        provider: "openid_connect",
+        uid: "user@example.net"
+      )
+    }
+
+    it { is_expected.to belong_to :user }
+    it { is_expected.to validate_uniqueness_of :uid }
+  end
+end

From 6c0d15b6f94f4a7504159c522d64bb01f47380ab Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 13 Feb 2024 16:21:53 +1100
Subject: [PATCH 05/13] Migrate existing OIDC account data

---
 ...3044159_copy_oidc_data_to_oidc_accounts.rb | 15 ++++++++
 db/schema.rb                                  |  2 +-
 ...59_copy_oidc_data_to_oidc_accounts_spec.rb | 35 +++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb
 create mode 100644 spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb

diff --git a/db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb b/db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb
new file mode 100644
index 0000000000..72b1d88d14
--- /dev/null
+++ b/db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class CopyOidcDataToOidcAccounts < ActiveRecord::Migration[7.0]
+  def up
+    execute <<~SQL.squish
+      INSERT INTO oidc_accounts (user_id, provider, uid, created_at, updated_at)
+      SELECT id, provider, uid, updated_at, updated_at
+      FROM spree_users WHERE provider IS NOT NULL
+    SQL
+  end
+
+  def down
+    execute "DELETE FROM oidc_accounts"
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 0aad1ebf69..0ab2fb2f2e 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_02_13_042618) do
+ActiveRecord::Schema[7.0].define(version: 2024_02_13_044159) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_stat_statements"
   enable_extension "plpgsql"
diff --git a/spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb b/spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb
new file mode 100644
index 0000000000..d2bbd81694
--- /dev/null
+++ b/spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../../db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts'
+
+describe CopyOidcDataToOidcAccounts do
+  describe "up" do
+    let!(:user) { create(:user) }
+    let!(:oidc_user) {
+      create(:user, provider: "openid_connect", uid: "ofn@example.net")
+    }
+
+    it "copies data" do
+      expect { subject.up }.to change {
+        OidcAccount.count
+      }.from(0).to(1)
+
+      account = OidcAccount.first
+
+      expect(account.user).to eq oidc_user
+      expect(account.provider).to eq oidc_user.provider
+      expect(account.uid).to eq oidc_user.uid
+      expect(account.token).to eq nil
+    end
+  end
+
+  describe "down" do
+    it "removes data" do
+      user = create(:user)
+      OidcAccount.create!(user:, provider: "oidc", uid: "ofn@exmpl.net")
+
+      expect { subject.down }.to change { OidcAccount.count }.from(1).to(0)
+    end
+  end
+end

From 07a86171439f0029e704332dd27060bb5e2f93e0 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 13 Feb 2024 17:04:21 +1100
Subject: [PATCH 06/13] Store OIDC account data in new model

---
 .../omniauth_callbacks_controller.rb          |  2 +-
 app/models/oidc_account.rb                    |  7 +++++
 app/models/spree/user.rb                      |  9 ------
 app/views/admin/oidc_settings/index.html.haml |  6 ++--
 .../app/services/authorization_control.rb     |  2 +-
 .../services/authorization_control_spec.rb    |  5 ++--
 spec/factories/user_factory.rb                |  2 +-
 spec/models/oidc_account_spec.rb              | 29 +++++++++++++++++++
 spec/models/spree/user_spec.rb                | 14 ---------
 .../omniauth_callbacks_controller_spec.rb     | 11 ++++---
 10 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index b3db1607ef..c483d6f4b7 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -2,7 +2,7 @@
 
 class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   def openid_connect
-    spree_current_user.link_from_omniauth(request.env["omniauth.auth"])
+    OidcAccount.link(spree_current_user, request.env["omniauth.auth"])
 
     redirect_to admin_oidc_settings_path
   end
diff --git a/app/models/oidc_account.rb b/app/models/oidc_account.rb
index 9d1eb8b752..1b90c9964b 100644
--- a/app/models/oidc_account.rb
+++ b/app/models/oidc_account.rb
@@ -6,4 +6,11 @@ class OidcAccount < ApplicationRecord
   # When a user authenticates via token, the `uid` should be mapped to only one
   # OFN user and therefore it needs to be unique.
   validates :uid, presence: true, uniqueness: true
+
+  def self.link(user, auth)
+    upsert_all(
+      [{user_id: user.id, provider: auth.provider, uid: auth.uid}],
+      unique_by: [:user_id]
+    )
+  end
 end
diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb
index fce0828b4c..b94c1ace9c 100644
--- a/app/models/spree/user.rb
+++ b/app/models/spree/user.rb
@@ -51,11 +51,6 @@ module Spree
 
     validates :email, 'valid_email_2/email': { mx: true }, if: :email_changed?
     validate :limit_owned_enterprises
-    validates :uid, uniqueness: true, if: lambda { uid.present? }
-
-    # Same validation as in the openid_connect gem.
-    # This validator is totally outdated but we indirectly depend on it.
-    validates :uid, email: true, if: lambda { uid.present? }
 
     class DestroyWithOrdersError < StandardError; end
 
@@ -63,10 +58,6 @@ module Spree
       User.admin.count > 0
     end
 
-    def link_from_omniauth(auth)
-      update!(provider: auth.provider, uid: auth.uid)
-    end
-
     # Whether a user has a role or not.
     def has_spree_role?(role_in_question)
       spree_roles.where(name: role_in_question.to_s).any?
diff --git a/app/views/admin/oidc_settings/index.html.haml b/app/views/admin/oidc_settings/index.html.haml
index eed72c78d2..b2ab3b3401 100644
--- a/app/views/admin/oidc_settings/index.html.haml
+++ b/app/views/admin/oidc_settings/index.html.haml
@@ -7,9 +7,11 @@
   %h2= t(".connect")
   %br
 
-  - if spree_current_user.provider == 'openid_connect' && spree_current_user.uid.present?
+  - # I'll refactor this later:
+  - account = OidcAccount.find_by(provider: "openid_connect", user: spree_current_user)
+  - if account
     = t(".already_connected")
-    = spree_current_user.uid
+    = account.uid
     %br
     %br
 
diff --git a/engines/dfc_provider/app/services/authorization_control.rb b/engines/dfc_provider/app/services/authorization_control.rb
index e2643b43fd..254c6cee67 100644
--- a/engines/dfc_provider/app/services/authorization_control.rb
+++ b/engines/dfc_provider/app/services/authorization_control.rb
@@ -57,6 +57,6 @@ class AuthorizationControl
   def find_ofn_user(payload)
     return if payload["email"].blank?
 
-    Spree::User.find_by(uid: payload["email"])
+    OidcAccount.find_by(uid: payload["email"])&.user
   end
 end
diff --git a/engines/dfc_provider/spec/services/authorization_control_spec.rb b/engines/dfc_provider/spec/services/authorization_control_spec.rb
index ed1f4cfe97..97cd646df3 100644
--- a/engines/dfc_provider/spec/services/authorization_control_spec.rb
+++ b/engines/dfc_provider/spec/services/authorization_control_spec.rb
@@ -16,9 +16,8 @@ describe AuthorizationControl do
     end
 
     it "ignores blank email" do
-      create(:user, uid: nil)
-      create(:user, uid: "")
-      token = allow_token_for(email: nil)
+      OidcAccount.where(user:).update_all(uid: "")
+      token = allow_token_for(email: "")
 
       expect(auth(oidc_token: token).user).to eq nil
     end
diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb
index e52d6c878d..b571ba259f 100644
--- a/spec/factories/user_factory.rb
+++ b/spec/factories/user_factory.rb
@@ -36,7 +36,7 @@ FactoryBot.define do
 
     factory :oidc_user do
       after(:create) do |user|
-        user.update uid: user.email
+        OidcAccount.create!(user:, provider: "openid_connect", uid: user.email)
       end
     end
   end
diff --git a/spec/models/oidc_account_spec.rb b/spec/models/oidc_account_spec.rb
index 4e70d550b2..936fea5dcc 100644
--- a/spec/models/oidc_account_spec.rb
+++ b/spec/models/oidc_account_spec.rb
@@ -15,4 +15,33 @@ describe OidcAccount, type: :model do
     it { is_expected.to belong_to :user }
     it { is_expected.to validate_uniqueness_of :uid }
   end
+
+  describe ".link" do
+    let(:user) { create(:user, email: "user@example.com") }
+    let(:auth) {
+      OmniAuth::AuthHash.new(
+        provider: "openid_connect",
+        uid: "user@example.net"
+      )
+    }
+
+    it "creates or updates an account record" do
+      expect { OidcAccount.link(user, auth) }
+        .to change { OidcAccount.count }.by(1)
+
+      account = OidcAccount.last
+      expect(account.user).to eq user
+      expect(account.provider).to eq "openid_connect"
+
+      auth.uid = "user@example.org"
+
+      expect {
+        OidcAccount.link(user, auth)
+        account.reload
+      }
+        .to change { OidcAccount.count }.by(0)
+        .and change { account.uid }
+        .from("user@example.net").to("user@example.org")
+    end
+  end
 end
diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb
index cc1284d050..8fc144b275 100644
--- a/spec/models/spree/user_spec.rb
+++ b/spec/models/spree/user_spec.rb
@@ -268,18 +268,4 @@ describe Spree::User do
       expect(user.disabled).to be_falsey
     end
   end
-
-  describe "#link_from_omniauth" do
-    let!(:user) { create(:user, email: "user@email.com") }
-    let(:auth) { double(:auth, provider: "openid_connect", uid: "user@email.com") }
-
-    it "creates a user without errors" do
-      user.link_from_omniauth(auth)
-
-      expect(user.errors.present?).to be false
-      expect(user.confirmed?).to be true
-      expect(user.provider).to eq "openid_connect"
-      expect(user.uid).to eq "user@email.com"
-    end
-  end
 end
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index 6c2034bfe7..0aad02a08c 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -28,10 +28,11 @@ describe '/user/spree_user/auth/openid_connect/callback', type: :request do
     end
 
     it 'is successful' do
-      request!
+      expect { request! }.to change { OidcAccount.count }.by(1)
 
-      expect(user.provider).to eq "openid_connect"
-      expect(user.uid).to eq "ofn@example.com"
+      account = OidcAccount.last
+      expect(account.provider).to eq "openid_connect"
+      expect(account.uid).to eq "ofn@example.com"
       expect(response.status).to eq(302)
     end
   end
@@ -42,10 +43,8 @@ describe '/user/spree_user/auth/openid_connect/callback', type: :request do
     end
 
     it 'fails with bad auth data' do
-      request!
+      expect { request! }.to_not change { OidcAccount.count }
 
-      expect(user.provider).to be_nil
-      expect(user.uid).to be_nil
       expect(response.status).to eq(302)
     end
   end

From 4f3ae4f2a474dd1ad6fe5c7dcc1df06ddd5af8a0 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 15 Feb 2024 10:55:42 +1100
Subject: [PATCH 07/13] Spec OIDC setup flow

---
 spec/system/admin/oidc_settings_spec.rb | 26 +++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 spec/system/admin/oidc_settings_spec.rb

diff --git a/spec/system/admin/oidc_settings_spec.rb b/spec/system/admin/oidc_settings_spec.rb
new file mode 100644
index 0000000000..18c62a87fe
--- /dev/null
+++ b/spec/system/admin/oidc_settings_spec.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'system_helper'
+
+describe "OIDC Settings" do
+  it "requires login" do
+    visit admin_oidc_settings_path
+    expect(page).to have_button "Login"
+    expect(page).to_not have_button "Link your Les Communs OIDC Account"
+  end
+
+  describe "with valid login" do
+    let(:user) { create(:admin_user) }
+
+    before do
+      OmniAuth.config.test_mode = true
+      login_as user
+    end
+
+    it "allows you to connect to an account" do
+      visit admin_oidc_settings_path
+      click_button "Link your Les Communs OIDC Account"
+      expect(page).to have_content "Your account is already linked"
+    end
+  end
+end

From a89b22e3973ae9fe221f6d58f6375e78b6c48b3d Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Wed, 14 Feb 2024 17:02:52 +1100
Subject: [PATCH 08/13] Allow user to disconnect OIDC account

This makes testing much easier. But probably also good for users to
revoke any access via OIDC apps. It also enables users to then connect
to a different account, or just renew the current connection.
---
 app/controllers/admin/oidc_settings_controller.rb | 9 ++++++++-
 app/models/spree/user.rb                          | 1 +
 app/views/admin/oidc_settings/index.html.haml     | 9 +++++----
 config/locales/en.yml                             | 1 +
 config/routes/admin.rb                            | 2 +-
 spec/system/admin/oidc_settings_spec.rb           | 5 ++++-
 6 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/app/controllers/admin/oidc_settings_controller.rb b/app/controllers/admin/oidc_settings_controller.rb
index f668442bc0..69ce9dbfc9 100644
--- a/app/controllers/admin/oidc_settings_controller.rb
+++ b/app/controllers/admin/oidc_settings_controller.rb
@@ -2,6 +2,13 @@
 
 module Admin
   class OidcSettingsController < Spree::Admin::BaseController
-    def index; end
+    def index
+      @account = spree_current_user.oidc_account
+    end
+
+    def destroy
+      spree_current_user.oidc_account&.destroy
+      redirect_to admin_oidc_settings_path
+    end
   end
 end
diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb
index b94c1ace9c..56c0607262 100644
--- a/app/models/spree/user.rb
+++ b/app/models/spree/user.rb
@@ -42,6 +42,7 @@ module Spree
     has_many :credit_cards, dependent: :destroy
     has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy
     has_many :webhook_endpoints, dependent: :destroy
+    has_one :oidc_account, dependent: :destroy
 
     accepts_nested_attributes_for :enterprise_roles, allow_destroy: true
     accepts_nested_attributes_for :webhook_endpoints
diff --git a/app/views/admin/oidc_settings/index.html.haml b/app/views/admin/oidc_settings/index.html.haml
index b2ab3b3401..1c33bb99bd 100644
--- a/app/views/admin/oidc_settings/index.html.haml
+++ b/app/views/admin/oidc_settings/index.html.haml
@@ -7,16 +7,17 @@
   %h2= t(".connect")
   %br
 
-  - # I'll refactor this later:
-  - account = OidcAccount.find_by(provider: "openid_connect", user: spree_current_user)
-  - if account
+  - if @account
     = t(".already_connected")
-    = account.uid
+    = @account.uid
     %br
     %br
 
     = t(".view_account")
     = link_to t(".les_communs_link"), "#{ Devise.omniauth_configs[:openid_connect].options[:issuer] }/account"
+    %br
+    %br
+    = button_to t(".disconnect"), admin_oidc_setting_path(@account), method: :delete
 
   - else
     = t(".link_your_account")
diff --git a/config/locales/en.yml b/config/locales/en.yml
index e615792f8c..9111bec265 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1712,6 +1712,7 @@ en:
       index:
         title: "OIDC Settings"
         connect: "Connect Your Account"
+        disconnect: "Disconnect"
         already_connected: "Your account is already linked to this DFC authorization account:"
         les_communs_link: "Les Communs Open ID server"
         link_your_account: "You need first to link your account with the authorization provider used by DFC (Les Communs Open ID Connect)."
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index b8fef0838c..20dace8c6a 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -113,7 +113,7 @@ Openfoodnetwork::Application.routes.draw do
       put :unpause, on: :member
     end
 
-    resources :oidc_settings, only: :index
+    resources :oidc_settings, only: [:index, :destroy]
 
     resources :subscription_line_items, only: [], format: :json do
       post :build, on: :collection
diff --git a/spec/system/admin/oidc_settings_spec.rb b/spec/system/admin/oidc_settings_spec.rb
index 18c62a87fe..2ea91b78b8 100644
--- a/spec/system/admin/oidc_settings_spec.rb
+++ b/spec/system/admin/oidc_settings_spec.rb
@@ -17,10 +17,13 @@ describe "OIDC Settings" do
       login_as user
     end
 
-    it "allows you to connect to an account" do
+    it "allows you to connect to an account and disconnect again" do
       visit admin_oidc_settings_path
       click_button "Link your Les Communs OIDC Account"
       expect(page).to have_content "Your account is already linked"
+
+      click_button "Disconnect"
+      expect(page).to have_button "Link your Les Communs OIDC Account"
     end
   end
 end

From 6d9c5a9c66560ac172c456ba51d04f0e0c3271e5 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 15 Feb 2024 13:09:11 +1100
Subject: [PATCH 09/13] Allow user to get new OIDC refresh token

The refresh token is usually valid for a year but it can be revoked at
any time. When we try to use it and it's expired, we should remove it
from the account record and notify the user. They can then refresh the
authorisation.
---
 app/models/oidc_account.rb                    | 12 ++++++++----
 app/views/admin/oidc_settings/index.html.haml | 10 ++++++++++
 config/locales/en.yml                         |  4 ++++
 spec/models/oidc_account_spec.rb              |  7 ++++---
 spec/system/admin/oidc_settings_spec.rb       | 13 +++++++++++++
 5 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/app/models/oidc_account.rb b/app/models/oidc_account.rb
index 1b90c9964b..84d6fd4a3f 100644
--- a/app/models/oidc_account.rb
+++ b/app/models/oidc_account.rb
@@ -8,9 +8,13 @@ class OidcAccount < ApplicationRecord
   validates :uid, presence: true, uniqueness: true
 
   def self.link(user, auth)
-    upsert_all(
-      [{user_id: user.id, provider: auth.provider, uid: auth.uid}],
-      unique_by: [:user_id]
-    )
+    attributes = {
+      user_id: user.id,
+      provider: auth.provider,
+      uid: auth.uid,
+      token: auth.dig(:credentials, :token),
+      refresh_token: auth.dig(:credentials, :refresh_token),
+    }
+    upsert_all([attributes], unique_by: [:user_id])
   end
 end
diff --git a/app/views/admin/oidc_settings/index.html.haml b/app/views/admin/oidc_settings/index.html.haml
index 1c33bb99bd..b35aeee8fe 100644
--- a/app/views/admin/oidc_settings/index.html.haml
+++ b/app/views/admin/oidc_settings/index.html.haml
@@ -19,6 +19,16 @@
     %br
     = button_to t(".disconnect"), admin_oidc_setting_path(@account), method: :delete
 
+    - if @account.refresh_token.blank?
+      %br
+      %br
+      %p= t(".note_expiry")
+      %br
+      %br
+      = button_to t(".refresh"),
+        Spree::Core::Engine.routes.url_helpers.spree_user_openid_connect_omniauth_authorize_path(auth_type: "login"),
+        data: { method: :post, "ujs-navigate": "false" }
+
   - else
     = t(".link_your_account")
     %br
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 9111bec265..c8e2dd6cad 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1717,6 +1717,10 @@ en:
         les_communs_link: "Les Communs Open ID server"
         link_your_account: "You need first to link your account with the authorization provider used by DFC (Les Communs Open ID Connect)."
         link_account_button: "Link your Les Communs OIDC Account"
+        note_expiry: |
+          Tokens to access connected apps have expired. Please refresh your
+          account connection to keep all integrations working.
+        refresh: "Refresh authorisation"
         view_account: "To view your account, see:"
     subscriptions:
       index:
diff --git a/spec/models/oidc_account_spec.rb b/spec/models/oidc_account_spec.rb
index 936fea5dcc..7d0ef6939e 100644
--- a/spec/models/oidc_account_spec.rb
+++ b/spec/models/oidc_account_spec.rb
@@ -20,8 +20,7 @@ describe OidcAccount, type: :model do
     let(:user) { create(:user, email: "user@example.com") }
     let(:auth) {
       OmniAuth::AuthHash.new(
-        provider: "openid_connect",
-        uid: "user@example.net"
+        JSON.parse(file_fixture("omniauth.auth.json").read)
       )
     }
 
@@ -32,6 +31,8 @@ describe OidcAccount, type: :model do
       account = OidcAccount.last
       expect(account.user).to eq user
       expect(account.provider).to eq "openid_connect"
+      expect(account.token).to match /^ey/
+      expect(account.refresh_token).to match /^ey/
 
       auth.uid = "user@example.org"
 
@@ -41,7 +42,7 @@ describe OidcAccount, type: :model do
       }
         .to change { OidcAccount.count }.by(0)
         .and change { account.uid }
-        .from("user@example.net").to("user@example.org")
+        .from("ofn@example.com").to("user@example.org")
     end
   end
 end
diff --git a/spec/system/admin/oidc_settings_spec.rb b/spec/system/admin/oidc_settings_spec.rb
index 2ea91b78b8..61e00bd8e1 100644
--- a/spec/system/admin/oidc_settings_spec.rb
+++ b/spec/system/admin/oidc_settings_spec.rb
@@ -25,5 +25,18 @@ describe "OIDC Settings" do
       click_button "Disconnect"
       expect(page).to have_button "Link your Les Communs OIDC Account"
     end
+
+    it "allows you to refresh authorisation tokens" do
+      OidcAccount.create!(user:, provider: "openid_connect", uid: "a@b.com")
+      OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new(
+        JSON.parse(file_fixture("omniauth.auth.json").read)
+      )
+
+      visit admin_oidc_settings_path
+
+      expect(page).to have_content "Tokens to access connected apps have expired"
+      click_button "Refresh authorisation"
+      expect(page).to_not have_content "Tokens to access connected apps have expired"
+    end
   end
 end

From a9b72c80958a5fb7903f68271828f54c2bb41842 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 15 Feb 2024 15:33:26 +1100
Subject: [PATCH 10/13] Comment on rare upsert usage

---
 app/models/oidc_account.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/oidc_account.rb b/app/models/oidc_account.rb
index 84d6fd4a3f..b755849753 100644
--- a/app/models/oidc_account.rb
+++ b/app/models/oidc_account.rb
@@ -15,6 +15,8 @@ class OidcAccount < ApplicationRecord
       token: auth.dig(:credentials, :token),
       refresh_token: auth.dig(:credentials, :refresh_token),
     }
-    upsert_all([attributes], unique_by: [:user_id])
+    # This skips validations but we have database constraints in place.
+    # We may replace this at some point.
+    upsert_all([attributes], unique_by: [:user_id]) # rubocop:disable Rails/SkipsModelValidations
   end
 end

From 0813f43b494f361c88a431d4f1085e8e5bae9c46 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 16 Feb 2024 11:53:28 +1100
Subject: [PATCH 11/13] Better wording for OIDC connection

---
 app/views/admin/oidc_settings/index.html.haml | 3 +--
 config/locales/en.yml                         | 2 +-
 spec/system/admin/oidc_settings_spec.rb       | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/app/views/admin/oidc_settings/index.html.haml b/app/views/admin/oidc_settings/index.html.haml
index b35aeee8fe..fac04aaec7 100644
--- a/app/views/admin/oidc_settings/index.html.haml
+++ b/app/views/admin/oidc_settings/index.html.haml
@@ -8,8 +8,7 @@
   %br
 
   - if @account
-    = t(".already_connected")
-    = @account.uid
+    = t(".connected", uid: @account.uid)
     %br
     %br
 
diff --git a/config/locales/en.yml b/config/locales/en.yml
index c8e2dd6cad..8d88ce2cd7 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1713,7 +1713,7 @@ en:
         title: "OIDC Settings"
         connect: "Connect Your Account"
         disconnect: "Disconnect"
-        already_connected: "Your account is already linked to this DFC authorization account:"
+        connected: "Your account is linked to %{uid}."
         les_communs_link: "Les Communs Open ID server"
         link_your_account: "You need first to link your account with the authorization provider used by DFC (Les Communs Open ID Connect)."
         link_account_button: "Link your Les Communs OIDC Account"
diff --git a/spec/system/admin/oidc_settings_spec.rb b/spec/system/admin/oidc_settings_spec.rb
index 61e00bd8e1..d4c0c8c151 100644
--- a/spec/system/admin/oidc_settings_spec.rb
+++ b/spec/system/admin/oidc_settings_spec.rb
@@ -20,7 +20,7 @@ describe "OIDC Settings" do
     it "allows you to connect to an account and disconnect again" do
       visit admin_oidc_settings_path
       click_button "Link your Les Communs OIDC Account"
-      expect(page).to have_content "Your account is already linked"
+      expect(page).to have_content "Your account is linked"
 
       click_button "Disconnect"
       expect(page).to have_button "Link your Les Communs OIDC Account"

From 4d8bb25f864ef649a277fa5bdc24ba22fd7d775f Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 20 Feb 2024 10:49:54 +1100
Subject: [PATCH 12/13] Allow enterprise users to disconnect their OIDC account

---
 app/models/spree/ability.rb             | 2 +-
 spec/factories/user_factory.rb          | 4 ++++
 spec/system/admin/oidc_settings_spec.rb | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb
index f4d776bc02..127b9c2766 100644
--- a/app/models/spree/ability.rb
+++ b/app/models/spree/ability.rb
@@ -177,7 +177,7 @@ module Spree
 
       can [:admin, :create], :manager_invitation
 
-      can [:admin, :index], :oidc_setting
+      can [:admin, :index, :destroy], :oidc_setting
 
       can [:admin, :create], Voucher
     end
diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb
index b571ba259f..b95fc16ead 100644
--- a/spec/factories/user_factory.rb
+++ b/spec/factories/user_factory.rb
@@ -30,6 +30,10 @@ FactoryBot.define do
       end
     end
 
+    factory :enterprise_user do
+      enterprises { [build(:enterprise)] }
+    end
+
     factory :admin_user do
       spree_roles { [Spree::Role.find_or_create_by!(name: 'admin')] }
     end
diff --git a/spec/system/admin/oidc_settings_spec.rb b/spec/system/admin/oidc_settings_spec.rb
index d4c0c8c151..160a9ca189 100644
--- a/spec/system/admin/oidc_settings_spec.rb
+++ b/spec/system/admin/oidc_settings_spec.rb
@@ -10,7 +10,7 @@ describe "OIDC Settings" do
   end
 
   describe "with valid login" do
-    let(:user) { create(:admin_user) }
+    let(:user) { create(:enterprise_user) }
 
     before do
       OmniAuth.config.test_mode = true

From 008e384f145e90d1f72161d4d87d345e957eada4 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 20 Feb 2024 11:02:14 +1100
Subject: [PATCH 13/13] Simplify oidc_user factory

Now you can build in memory only as well.
---
 spec/factories/user_factory.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb
index b95fc16ead..80f2ecf432 100644
--- a/spec/factories/user_factory.rb
+++ b/spec/factories/user_factory.rb
@@ -39,9 +39,9 @@ FactoryBot.define do
     end
 
     factory :oidc_user do
-      after(:create) do |user|
-        OidcAccount.create!(user:, provider: "openid_connect", uid: user.email)
-      end
+      oidc_account {
+        OidcAccount.new(provider: "openid_connect", uid: email)
+      }
     end
   end
 end