Merge pull request #12160 from mkllnk/oidc-tokens

Store OIDC tokens to call DFC APIs

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

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

app/models/oidc_account.rb

@@ -0,0 +1,22 @@
+# 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
+
+  def self.link(user, auth)
+    attributes = {
+      user_id: user.id,
+      provider: auth.provider,
+      uid: auth.uid,
+      token: auth.dig(:credentials, :token),
+      refresh_token: auth.dig(:credentials, :refresh_token),
+    }
+    # 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

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

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
@@ -51,11 +52,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 +59,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?

app/views/admin/oidc_settings/index.html.haml

@@ -7,14 +7,26 @@
   %h2= t(".connect")
   %br
 
-  - if spree_current_user.provider == 'openid_connect' && spree_current_user.uid.present?
-    = t(".already_connected")
-    = spree_current_user.uid
+  - if @account
+    = t(".connected", 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
+
+    - 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")

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

config/locales/en.yml

@@ -1712,10 +1712,15 @@ en:
       index:
         title: "OIDC Settings"
         connect: "Connect Your Account"
-        already_connected: "Your account is already linked to this DFC authorization account:"
+        disconnect: "Disconnect"
+        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"
+        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:

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

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

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

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

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

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

spec/factories/user_factory.rb

@@ -30,14 +30,18 @@ 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
 
     factory :oidc_user do
-      after(:create) do |user|
-        user.update uid: user.email
-      end
+      oidc_account {
+        OidcAccount.new(provider: "openid_connect", uid: email)
+      }
     end
   end
 end

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

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

spec/models/oidc_account_spec.rb

@@ -0,0 +1,48 @@
+# 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
+
+  describe ".link" do
+    let(:user) { create(:user, email: "user@example.com") }
+    let(:auth) {
+      OmniAuth::AuthHash.new(
+        JSON.parse(file_fixture("omniauth.auth.json").read)
+      )
+    }
+
+    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"
+      expect(account.token).to match /^ey/
+      expect(account.refresh_token).to match /^ey/
+
+      auth.uid = "user@example.org"
+
+      expect {
+        OidcAccount.link(user, auth)
+        account.reload
+      }
+        .to change { OidcAccount.count }.by(0)
+        .and change { account.uid }
+        .from("ofn@example.com").to("user@example.org")
+    end
+  end
+end

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

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -2,55 +2,49 @@
 
 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' }
-
-    request.env['devise.mapping'] = Devise.mappings[:spree_user]
-    request.env['omniauth.auth'] = omniauth_response
   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
+    # 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
 
     it 'is successful' do
-      subject
+      expect { request! }.to change { OidcAccount.count }.by(1)
 
-      expect(user.provider).to eq "openid_connect"
-      expect(user.uid).to eq "john@doe.com"
-      expect(request.cookies[:omniauth_connect]).to be_nil
+      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
 
   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
+      expect { request! }.to_not change { OidcAccount.count }
 
-      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

spec/system/admin/oidc_settings_spec.rb

@@ -0,0 +1,42 @@
+# 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(:enterprise_user) }
+
+    before do
+      OmniAuth.config.test_mode = true
+      login_as user
+    end
+
+    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 linked"
+
+      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