From 1ee20ab3a6faaa339e2bf31acbcba753de561e16 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Sat, 25 Oct 2025 22:23:28 +0200
Subject: [PATCH] Eliminate code duplication in OIDC identity creation (#230)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Eliminate duplication by using create_from_omniauth method

- Updated OidcIdentity.create_from_omniauth to set last_authenticated_at
- Refactored OidcAccountsController to use create_from_omniauth instead of direct create! calls
- Updated test to verify last_authenticated_at is set by create_from_omniauth

Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>

* Extract auth hash building into private helper method

- Added build_auth_hash helper method to eliminate OpenStruct creation duplication
- Both create_link and create_user actions now use the same helper

Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>

* Linter fix

* Fix button style on OIDC link step

* Fix dark mode styles

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
---
 app/controllers/oidc_accounts_controller.rb | 37 ++++++++++-----------
 app/models/oidc_identity.rb                 |  3 +-
 app/views/oidc_accounts/link.html.erb       | 22 +++++++++---
 test/models/oidc_identity_test.rb           |  1 +
 4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/app/controllers/oidc_accounts_controller.rb b/app/controllers/oidc_accounts_controller.rb
index 231790ab3..b6d7466bd 100644
--- a/app/controllers/oidc_accounts_controller.rb
+++ b/app/controllers/oidc_accounts_controller.rb
@@ -28,16 +28,9 @@ class OidcAccountsController < ApplicationController
 
     if user
       # Create the OIDC identity link
-      oidc_identity = user.oidc_identities.create!(
-        provider: @pending_auth["provider"],
-        uid: @pending_auth["uid"],
-        info: {
-          email: @pending_auth["email"],
-          name: @pending_auth["name"],
-          first_name: @pending_auth["first_name"],
-          last_name: @pending_auth["last_name"]
-        },
-        last_authenticated_at: Time.current
+      oidc_identity = OidcIdentity.create_from_omniauth(
+        build_auth_hash(@pending_auth),
+        user
       )
 
       # Clear pending auth from session
@@ -100,16 +93,9 @@ class OidcAccountsController < ApplicationController
 
     if @user.save
       # Create the OIDC identity
-      @user.oidc_identities.create!(
-        provider: @pending_auth["provider"],
-        uid: @pending_auth["uid"],
-        info: {
-          email: @pending_auth["email"],
-          name: @pending_auth["name"],
-          first_name: @pending_auth["first_name"],
-          last_name: @pending_auth["last_name"]
-        },
-        last_authenticated_at: Time.current
+      OidcIdentity.create_from_omniauth(
+        build_auth_hash(@pending_auth),
+        @user
       )
 
       # Clear pending auth from session
@@ -122,4 +108,15 @@ class OidcAccountsController < ApplicationController
       render :new_user, status: :unprocessable_entity
     end
   end
+
+  private
+
+    # Convert pending auth hash to OmniAuth-like structure
+    def build_auth_hash(pending_auth)
+      OpenStruct.new(
+        provider: pending_auth["provider"],
+        uid: pending_auth["uid"],
+        info: OpenStruct.new(pending_auth.slice("email", "name", "first_name", "last_name"))
+      )
+    end
 end
diff --git a/app/models/oidc_identity.rb b/app/models/oidc_identity.rb
index 12b8fd477..78276546d 100644
--- a/app/models/oidc_identity.rb
+++ b/app/models/oidc_identity.rb
@@ -21,7 +21,8 @@ class OidcIdentity < ApplicationRecord
         name: auth.info&.name,
         first_name: auth.info&.first_name,
         last_name: auth.info&.last_name
-      }
+      },
+      last_authenticated_at: Time.current
     )
   end
 end
diff --git a/app/views/oidc_accounts/link.html.erb b/app/views/oidc_accounts/link.html.erb
index bfab765ec..dbed9f2cd 100644
--- a/app/views/oidc_accounts/link.html.erb
+++ b/app/views/oidc_accounts/link.html.erb
@@ -42,21 +42,33 @@
   </div>
 
   <div class="space-y-4">
-    <div class="p-4 bg-gray-50 border border-gray-200 rounded-md">
-      <p class="text-sm text-gray-700">
+    <div class="p-4 bg-container border border-secondary rounded-md">
+      <p class="text-sm text-primary">
         <strong>Email:</strong> <%= @pending_auth["email"] %>
       </p>
       <% if @pending_auth["name"].present? %>
-        <p class="text-sm text-gray-700 mt-2">
+        <p class="text-sm text-primary mt-2">
           <strong>Name:</strong> <%= @pending_auth["name"] %>
         </p>
       <% end %>
     </div>
 
-    <%= button_to "Create Account", create_user_oidc_account_path, method: :post, class: "w-full", data: { turbo: false } %>
+    <%= render DS::Button.new(
+      text: "Create Account",
+      href: create_user_oidc_account_path,
+      full_width: true,
+      variant: :primary,
+      method: :post,
+      data: { turbo: false }
+    ) %>
   </div>
 <% end %>
 
 <div class="mt-6 text-center">
-  <%= link_to "Cancel", new_session_path, class: "font-medium text-sm text-primary hover:underline transition" %>
+  <%= render DS::Link.new(
+    text: "Cancel",
+    href: new_session_path,
+    variant: :default,
+    class: "font-medium text-sm text-primary hover:underline transition"
+  ) %>
 </div>
diff --git a/test/models/oidc_identity_test.rb b/test/models/oidc_identity_test.rb
index 09d83902b..43342964b 100644
--- a/test/models/oidc_identity_test.rb
+++ b/test/models/oidc_identity_test.rb
@@ -77,5 +77,6 @@ class OidcIdentityTest < ActiveSupport::TestCase
     assert_equal "test@example.com", identity.info["email"]
     assert_equal "Test User", identity.info["name"]
     assert_equal @user, identity.user
+    assert_not_nil identity.last_authenticated_at
   end
 end