From d94842e941ad169012adedd9ce0d92a3cab3feab Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Sat, 7 Dec 2024 13:59:54 +1000
Subject: [PATCH 01/11] Add API endpoints and services for Account Ownership
 Verification PIN

Introduces new API endpoints and supporting services for the
Account Ownership Verification PIN feature. This allows users
to verify ownership of the account by generating and providing
the unique PIN number. The pin has a short expiry and is only
stored in Redis.

The changes include:
- New POST endpoint to create a security PIN for authenticated users
- New GET endpoint for users to retrieve their current security PIN
- New GET endpoint for admins to retrieve a user's security PIN by user ID
- Implement Users::SecurityPin::CreateService for PIN generation
- Implement Users::SecurityPin::RetrieveService for PIN retrieval
- Add Redis integration for temporary PIN storage

These changes provide the backend support necessary for implementing
the Account Ownership Verification PIN feature, allowing for secure
account verification during support interactions.

Changelog: added
Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/233164
---
 .../users/support_pin/base_service.rb         | 19 ++++++
 .../users/support_pin/retrieve_service.rb     | 17 +++++
 .../users/support_pin/update_service.rb       | 32 ++++++++++
 lib/api/entities/user_support_pin.rb          | 11 ++++
 lib/api/users.rb                              | 62 +++++++++++++++++++
 5 files changed, 141 insertions(+)
 create mode 100644 app/services/users/support_pin/base_service.rb
 create mode 100644 app/services/users/support_pin/retrieve_service.rb
 create mode 100644 app/services/users/support_pin/update_service.rb
 create mode 100644 lib/api/entities/user_support_pin.rb

diff --git a/app/services/users/support_pin/base_service.rb b/app/services/users/support_pin/base_service.rb
new file mode 100644
index 00000000000000..567db6ab78e90c
--- /dev/null
+++ b/app/services/users/support_pin/base_service.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Users
+  module SupportPin
+    class BaseService
+      def initialize(user)
+        @user = user
+      end
+
+      def pin_key
+        "support_pin:#{@user.id}"
+      end
+
+      def pin_expiration_time
+        24.hours.from_now
+      end
+    end
+  end
+end
diff --git a/app/services/users/support_pin/retrieve_service.rb b/app/services/users/support_pin/retrieve_service.rb
new file mode 100644
index 00000000000000..3e730399137e7f
--- /dev/null
+++ b/app/services/users/support_pin/retrieve_service.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module Users
+  module SupportPin
+    class RetrieveService < SupportPin::BaseService
+      def execute
+        Gitlab::Redis::Cache.with do |redis|
+          key = pin_key
+          pin = redis.get(key)
+          expires_at = redis.ttl(key)
+
+          { pin: pin, expires_at: Time.zone.now + expires_at } if pin && expires_at > 0
+        end
+      end
+    end
+  end
+end
diff --git a/app/services/users/support_pin/update_service.rb b/app/services/users/support_pin/update_service.rb
new file mode 100644
index 00000000000000..176a8902dfeb4c
--- /dev/null
+++ b/app/services/users/support_pin/update_service.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module Users
+  module SupportPin
+    class UpdateService < SupportPin::BaseService
+      def execute
+        pin = generate_pin
+        expiration = pin_expiration_time
+
+        if store_pin(pin, expiration)
+          { status: :success, pin: pin, expires_at: expiration }
+        else
+          { status: :error, message: 'Failed to create support PIN' }
+        end
+      end
+
+      private
+
+      def generate_pin
+        SecureRandom.random_number(100000..999999).to_s
+      end
+
+      def store_pin(pin, expiration)
+        Gitlab::Redis::Cache.with do |redis|
+          key = pin_key
+          redis.set(key, pin)
+          redis.expireat(key, expiration.to_i)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/api/entities/user_support_pin.rb b/lib/api/entities/user_support_pin.rb
new file mode 100644
index 00000000000000..11a40e583d34db
--- /dev/null
+++ b/lib/api/entities/user_support_pin.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module API
+  module Entities
+    class UserSupportPin < Grape::Entity
+      expose :pin, documentation: { type: 'string', desc: 'The security PIN' }
+      expose :expires_at,
+        documentation: { type: 'string', format: 'date-time', desc: 'The expiration time of the PIN' }
+    end
+  end
+end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index eb0ca097ca8599..a0341cab620497 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -101,6 +101,33 @@ def reorder_users(users)
         end
       end
 
+      desc 'Get support PIN for a user. Available only for admins.' do
+        detail 'This feature allows administrators to retrieve the support PIN for a specified user'
+        success Entities::UserSupportPin
+        is_array false
+      end
+      params do
+        requires :id, type: Integer, desc: 'The ID of the user'
+      end
+      get ":id/support_pin", feature_category: :user_management do
+        authenticated_as_admin!
+
+        user = User.find_by_id(params[:id])
+        not_found!('User') unless user
+
+        begin
+          result = ::Users::SupportPin::RetrieveService.new(user).execute
+        rescue StandardError
+          error!("lol", :unprocessable_entity)
+        end
+
+        if result
+          present result, with: Entities::UserSupportPin
+        else
+          not_found!('Support PIN not found or expired')
+        end
+      end
+
       desc 'Get the list of users' do
         success Entities::UserBasic
       end
@@ -1333,6 +1360,41 @@ def set_user_status(include_missing_params:)
         end
       end
 
+      desc 'Create a new Support PIN for the authenticated user' do
+        detail 'This feature creates a temporary Support PIN that expires in one hour'
+        success Entities::UserSupportPin
+      end
+      post "support_pin", feature_category: :user_profile do
+        authenticate!
+
+        result = ::Users::SupportPin::UpdateService.new(current_user).execute
+
+        if result[:status] == :success
+          present({ pin: result[:pin], expires_at: result[:expires_at] }, with: Entities::UserSupportPin)
+        else
+          error!(result[:message], :unprocessable_entity)
+        end
+      end
+
+      desc 'Get the current Support PIN for the authenticated user' do
+        detail 'This feature retrieves the temporary Support PIN that expires in one hour'
+        success Entities::UserSupportPin
+      end
+      get "support_pin", feature_category: :user_profile do
+        authenticate!
+
+        result = ::Users::SupportPin::RetrieveService.new(current_user).execute
+
+        if result
+          # Convert the Time object to ISO 8601 format
+          expires_at = result[:expires_at].iso8601
+
+          present({ pin: result[:pin], expires_at: expires_at }, with: Entities::UserSupportPin)
+        else
+          not_found!('Support PIN not found or expired')
+        end
+      end
+
       desc "Update the current user's preferences" do
         success Entities::UserPreferences
         detail 'This feature was introduced in GitLab 13.10.'
-- 
GitLab


From 1241070d71cfcbe308e3df30f9fa4e08d5b6d643 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Mon, 9 Dec 2024 15:32:54 +1000
Subject: [PATCH 02/11] Add rspec tests for support pin API and services

---
 spec/requests/api/users_spec.rb               | 68 +++++++++++++++++++
 .../support_pin/retrieve_service.spec.rb      | 27 ++++++++
 .../users/support_pin/update_service_spec.rb  | 31 +++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 spec/services/users/support_pin/retrieve_service.spec.rb
 create mode 100644 spec/services/users/support_pin/update_service_spec.rb

diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 226702b0724221..ff5fcb40a53461 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -24,6 +24,7 @@
   let(:internal_user) { create(:user, :bot) }
   let(:user_with_2fa) { create(:user, :two_factor_via_otp) }
   let(:admin_with_2fa) { create(:admin, :two_factor_via_otp) }
+  let(:user_without_pin) { create(:user) }
 
   context 'admin notes' do
     let_it_be(:admin) { create(:admin, note: '2019-10-06 | 2FA added | user requested | www.gitlab.com') }
@@ -5391,4 +5392,71 @@ def update_password(user, admin, password = User.random_password)
     let(:attributable) { user }
     let(:other_attributable) { admin }
   end
+
+  describe 'POST /api/v4/user/support_pin' do
+
+    context 'when authenticated' do  
+      it 'creates a new support PIN' do
+        post api('/user/support_pin', user)
+
+        expect(response).to have_http_status(:created)
+        expect(json_response).to include('pin', 'expires_at')
+      end
+    end
+
+    context 'when not authenticated' do
+      it 'returns 401 Unauthorized' do
+        post api('/user/support_pin')
+
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+  end
+
+  describe 'GET /api/v4/user/support_pin' do
+    context 'when authenticated' do
+
+      it 'retrieves the current support PIN' do
+        get api('/user/support_pin', user)
+    
+        expect(response).to have_http_status(:ok)
+        expect(json_response).to include('pin', 'expires_at')
+      end
+
+      it 'returns 404 Not Found when no PIN exists' do
+        
+        get api('/user/support_pin', user_without_pin)
+
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+  end
+
+  describe 'GET /api/v4/users/:id/support_pin' do
+    context 'when authenticated as admin' do
+
+      it 'retrieves the support PIN for a user' do
+
+        get api("/users/#{user.id}/support_pin", admin, admin_mode: true)
+
+        expect(response).to have_http_status(:ok)
+        expect(json_response).to include('pin', 'expires_at')
+      end
+
+      it 'returns 404 Not Found when no PIN exists' do
+        get api("/users/#{user_without_pin.id}/support_pin", admin, admin_mode: true)
+
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context 'when authenticated as non-admin' do
+      it 'returns 403 Forbidden' do
+        get api("/users/#{user.id}/support_pin", user)
+
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+  end
+
 end
diff --git a/spec/services/users/support_pin/retrieve_service.spec.rb b/spec/services/users/support_pin/retrieve_service.spec.rb
new file mode 100644
index 00000000000000..7baf4bf617c776
--- /dev/null
+++ b/spec/services/users/support_pin/retrieve_service.spec.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::SupportPin::RetrieveService do
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(user) }
+
+  describe '#execute' do
+    context 'when a PIN exists' do
+      let!(:pin) { Users::SupportPin::UpdateService.new(user).execute[:pin] }
+
+      it 'retrieves the existing PIN' do
+        result = service.execute
+
+        expect(result[:pin]).to eq(pin)
+        expect(result[:expires_at]).to be_within(1.minute).of(24.hour.from_now)
+      end
+    end
+
+    context 'when no PIN exists' do
+      it 'returns nil' do
+        expect(service.execute).to be_nil
+      end
+    end
+  end
+end
diff --git a/spec/services/users/support_pin/update_service_spec.rb b/spec/services/users/support_pin/update_service_spec.rb
new file mode 100644
index 00000000000000..477bb4e9e3fc16
--- /dev/null
+++ b/spec/services/users/support_pin/update_service_spec.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::SupportPin::UpdateService do
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(user) }
+
+  describe '#execute' do
+    it 'creates a new support PIN' do
+      result = service.execute
+
+      expect(result[:status]).to eq(:success)
+      expect(result[:pin]).to match(/^\d{6}$/)
+      expect(result[:expires_at]).to be_within(1.minute).of(24.hour.from_now)
+    end
+
+  
+
+    it 'stores the PIN in Redis' do
+      result = service.execute
+      key = "support_pin:#{user.id}"
+      pin = result[:pin]
+
+      Gitlab::Redis::Cache.with do |redis|
+        expect(redis.get(key)).to eq(pin)
+      end
+    end
+
+  end
+end
-- 
GitLab


From 81c5799d4b853a4932780fae8ec10d643de54520 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Mon, 9 Dec 2024 16:06:34 +1000
Subject: [PATCH 03/11] Fix lint and rubocop errors

---
 spec/requests/api/users_spec.rb               | 24 +++++++------------
 ...rvice.spec.rb => retrieve_service_spec.rb} |  2 +-
 .../users/support_pin/update_service_spec.rb  |  7 ++----
 3 files changed, 12 insertions(+), 21 deletions(-)
 rename spec/services/users/support_pin/{retrieve_service.spec.rb => retrieve_service_spec.rb} (97%)

diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index ff5fcb40a53461..120f3d52dea05c 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -5394,12 +5394,11 @@ def update_password(user, admin, password = User.random_password)
   end
 
   describe 'POST /api/v4/user/support_pin' do
-
-    context 'when authenticated' do  
+    context 'when authenticated' do
       it 'creates a new support PIN' do
         post api('/user/support_pin', user)
 
-        expect(response).to have_http_status(:created)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response).to include('pin', 'expires_at')
       end
     end
@@ -5408,45 +5407,41 @@ def update_password(user, admin, password = User.random_password)
       it 'returns 401 Unauthorized' do
         post api('/user/support_pin')
 
-        expect(response).to have_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
   end
 
   describe 'GET /api/v4/user/support_pin' do
     context 'when authenticated' do
-
       it 'retrieves the current support PIN' do
         get api('/user/support_pin', user)
-    
-        expect(response).to have_http_status(:ok)
+
+        expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to include('pin', 'expires_at')
       end
 
       it 'returns 404 Not Found when no PIN exists' do
-        
         get api('/user/support_pin', user_without_pin)
 
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
   end
 
   describe 'GET /api/v4/users/:id/support_pin' do
     context 'when authenticated as admin' do
-
       it 'retrieves the support PIN for a user' do
-
         get api("/users/#{user.id}/support_pin", admin, admin_mode: true)
 
-        expect(response).to have_http_status(:ok)
+        expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to include('pin', 'expires_at')
       end
 
       it 'returns 404 Not Found when no PIN exists' do
         get api("/users/#{user_without_pin.id}/support_pin", admin, admin_mode: true)
 
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
@@ -5454,9 +5449,8 @@ def update_password(user, admin, password = User.random_password)
       it 'returns 403 Forbidden' do
         get api("/users/#{user.id}/support_pin", user)
 
-        expect(response).to have_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
   end
-
 end
diff --git a/spec/services/users/support_pin/retrieve_service.spec.rb b/spec/services/users/support_pin/retrieve_service_spec.rb
similarity index 97%
rename from spec/services/users/support_pin/retrieve_service.spec.rb
rename to spec/services/users/support_pin/retrieve_service_spec.rb
index 7baf4bf617c776..9d61ebafed5afa 100644
--- a/spec/services/users/support_pin/retrieve_service.spec.rb
+++ b/spec/services/users/support_pin/retrieve_service_spec.rb
@@ -14,7 +14,7 @@
         result = service.execute
 
         expect(result[:pin]).to eq(pin)
-        expect(result[:expires_at]).to be_within(1.minute).of(24.hour.from_now)
+        expect(result[:expires_at]).to be_within(1.minute).of(24.hours.from_now)
       end
     end
 
diff --git a/spec/services/users/support_pin/update_service_spec.rb b/spec/services/users/support_pin/update_service_spec.rb
index 477bb4e9e3fc16..328427cb92e99d 100644
--- a/spec/services/users/support_pin/update_service_spec.rb
+++ b/spec/services/users/support_pin/update_service_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Users::SupportPin::UpdateService do
+RSpec.describe Users::SupportPin::UpdateService, feature_category: :user_management do
   let(:user) { create(:user) }
   let(:service) { described_class.new(user) }
 
@@ -12,11 +12,9 @@
 
       expect(result[:status]).to eq(:success)
       expect(result[:pin]).to match(/^\d{6}$/)
-      expect(result[:expires_at]).to be_within(1.minute).of(24.hour.from_now)
+      expect(result[:expires_at]).to be_within(1.minute).of(24.hours.from_now)
     end
 
-  
-
     it 'stores the PIN in Redis' do
       result = service.execute
       key = "support_pin:#{user.id}"
@@ -26,6 +24,5 @@
         expect(redis.get(key)).to eq(pin)
       end
     end
-
   end
 end
-- 
GitLab


From f3bb24d43e38b5a173535ea56022de9d9586d15f Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Mon, 9 Dec 2024 17:39:07 +1000
Subject: [PATCH 04/11] Add feature category to rspec test

---
 spec/services/users/support_pin/retrieve_service_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/services/users/support_pin/retrieve_service_spec.rb b/spec/services/users/support_pin/retrieve_service_spec.rb
index 9d61ebafed5afa..b9b5b2057f8cdd 100644
--- a/spec/services/users/support_pin/retrieve_service_spec.rb
+++ b/spec/services/users/support_pin/retrieve_service_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Users::SupportPin::RetrieveService do
+RSpec.describe Users::SupportPin::RetrieveService, feature_category: :user_management do
   let(:user) { create(:user) }
   let(:service) { described_class.new(user) }
 
-- 
GitLab


From 4f078cba5ce952f78941292f8ccfa545c5717978 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Wed, 11 Dec 2024 14:03:00 +1000
Subject: [PATCH 05/11] Add support pin methods to User model

- Add support_pin_data, support_pin, support_pin_expires_at
- Add RSpec tests for new methods
- Implement memoization to minimize service calls

This change supports the Account Ownership Verification PIN
feature by providing easy access to support pin information
for authenticated users.
---
 app/models/user.rb       | 14 +++++++++++
 spec/models/user_spec.rb | 50 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/app/models/user.rb b/app/models/user.rb
index 3b2d6faad69be0..fc182378ed8956 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2558,6 +2558,20 @@ def uploads_sharding_key
   def add_admin_note(new_note)
     self.note = "#{new_note}\n#{self.note}"
   end
+  
+  # rubocop: disable CodeReuse/ServiceClass
+  def support_pin_data
+    @support_pin_data ||= Users::SupportPin::RetrieveService.new(self).execute
+  end
+  # rubocop: enable CodeReuse/ServiceClass
+
+  def support_pin
+    support_pin_data&.fetch(:pin, nil)
+  end
+
+  def support_pin_expires_at
+    support_pin_data&.fetch(:expires_at, nil)
+  end
 
   protected
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 2ea45caa503e70..db10e5aafd2bd0 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -9181,4 +9181,54 @@ def owner_class_attribute_default; end
       expect(user.uploads_sharding_key).to eq({})
     end
   end
+
+  describe 'support pin methods' do
+    let(:user) { create(:user) }
+    let(:pin_data) { { pin: '123456', expires_at: 1.hour.from_now } }
+    let(:retrieve_service) { instance_double(Users::SupportPin::RetrieveService) }
+
+    before do
+      allow(Users::SupportPin::RetrieveService).to receive(:new).with(user).and_return(retrieve_service)
+    end
+
+    describe '#support_pin' do
+      it 'returns the pin when it exists' do
+        allow(retrieve_service).to receive(:execute).and_return(pin_data)
+        expect(user.support_pin).to eq('123456')
+      end
+
+      it 'returns nil when no pin exists' do
+        allow(retrieve_service).to receive(:execute).and_return(nil)
+        expect(user.support_pin).to be_nil
+      end
+
+      it 'returns nil when pin key is missing' do
+        allow(retrieve_service).to receive(:execute).and_return({})
+        expect(user.support_pin).to be_nil
+      end
+    end
+
+    describe '#support_pin_expires_at' do
+      it 'returns the expiration time when it exists' do
+        allow(retrieve_service).to receive(:execute).and_return(pin_data)
+        expect(user.support_pin_expires_at).to eq(pin_data[:expires_at])
+      end
+
+      it 'returns nil when no expiration time exists' do
+        allow(retrieve_service).to receive(:execute).and_return(nil)
+        expect(user.support_pin_expires_at).to be_nil
+      end
+
+      it 'returns nil when expires_at key is missing' do
+        allow(retrieve_service).to receive(:execute).and_return({})
+        expect(user.support_pin_expires_at).to be_nil
+      end
+    end
+
+    it 'only calls the retrieve service once for multiple method calls' do
+      expect(retrieve_service).to receive(:execute).once.and_return(pin_data)
+      user.support_pin
+      user.support_pin_expires_at
+    end
+  end
 end
-- 
GitLab


From 618e6495317bf4dab9ec1246b6f310a94e40d4f1 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Wed, 18 Dec 2024 09:39:59 +1000
Subject: [PATCH 06/11] Update expiration date to 7 days

Based on discussion, the key expiration is now 7 days.
This commit also changes some of the base service methods
to constants, to allow them to be referenced elsewhere.
---
 app/services/users/support_pin/base_service.rb           | 9 ++++-----
 app/services/users/support_pin/update_service.rb         | 2 +-
 lib/api/users.rb                                         | 4 ++--
 spec/models/user_spec.rb                                 | 2 +-
 spec/services/users/support_pin/retrieve_service_spec.rb | 2 +-
 spec/services/users/support_pin/update_service_spec.rb   | 2 +-
 6 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/app/services/users/support_pin/base_service.rb b/app/services/users/support_pin/base_service.rb
index 567db6ab78e90c..cc363c38403a19 100644
--- a/app/services/users/support_pin/base_service.rb
+++ b/app/services/users/support_pin/base_service.rb
@@ -3,16 +3,15 @@
 module Users
   module SupportPin
     class BaseService
+      SUPPORT_PIN_PREFIX = "support_pin"
+      SUPPORT_PIN_EXPIRATION = 7.days.from_now
+
       def initialize(user)
         @user = user
       end
 
       def pin_key
-        "support_pin:#{@user.id}"
-      end
-
-      def pin_expiration_time
-        24.hours.from_now
+        "#{SUPPORT_PIN_PREFIX}:#{@user.id}"
       end
     end
   end
diff --git a/app/services/users/support_pin/update_service.rb b/app/services/users/support_pin/update_service.rb
index 176a8902dfeb4c..ee716957296fb2 100644
--- a/app/services/users/support_pin/update_service.rb
+++ b/app/services/users/support_pin/update_service.rb
@@ -5,7 +5,7 @@ module SupportPin
     class UpdateService < SupportPin::BaseService
       def execute
         pin = generate_pin
-        expiration = pin_expiration_time
+        expiration = SUPPORT_PIN_EXPIRATION
 
         if store_pin(pin, expiration)
           { status: :success, pin: pin, expires_at: expiration }
diff --git a/lib/api/users.rb b/lib/api/users.rb
index a0341cab620497..aefdb2bba2ad98 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -1361,7 +1361,7 @@ def set_user_status(include_missing_params:)
       end
 
       desc 'Create a new Support PIN for the authenticated user' do
-        detail 'This feature creates a temporary Support PIN that expires in one hour'
+        detail 'This feature creates a temporary Support PIN for the authenticated user'
         success Entities::UserSupportPin
       end
       post "support_pin", feature_category: :user_profile do
@@ -1377,7 +1377,7 @@ def set_user_status(include_missing_params:)
       end
 
       desc 'Get the current Support PIN for the authenticated user' do
-        detail 'This feature retrieves the temporary Support PIN that expires in one hour'
+        detail 'This feature retrieves the temporary Support PIN for the authenticated user'
         success Entities::UserSupportPin
       end
       get "support_pin", feature_category: :user_profile do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index db10e5aafd2bd0..08aa88e76aaefe 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -9184,7 +9184,7 @@ def owner_class_attribute_default; end
 
   describe 'support pin methods' do
     let(:user) { create(:user) }
-    let(:pin_data) { { pin: '123456', expires_at: 1.hour.from_now } }
+    let(:pin_data) { { pin: '123456', expires_at: 7.days.from_now } }
     let(:retrieve_service) { instance_double(Users::SupportPin::RetrieveService) }
 
     before do
diff --git a/spec/services/users/support_pin/retrieve_service_spec.rb b/spec/services/users/support_pin/retrieve_service_spec.rb
index b9b5b2057f8cdd..e867e87fd5c04d 100644
--- a/spec/services/users/support_pin/retrieve_service_spec.rb
+++ b/spec/services/users/support_pin/retrieve_service_spec.rb
@@ -14,7 +14,7 @@
         result = service.execute
 
         expect(result[:pin]).to eq(pin)
-        expect(result[:expires_at]).to be_within(1.minute).of(24.hours.from_now)
+        expect(result[:expires_at]).to be_within(1.minute).of(described_class::SUPPORT_PIN_EXPIRATION)
       end
     end
 
diff --git a/spec/services/users/support_pin/update_service_spec.rb b/spec/services/users/support_pin/update_service_spec.rb
index 328427cb92e99d..bd67f897fe705e 100644
--- a/spec/services/users/support_pin/update_service_spec.rb
+++ b/spec/services/users/support_pin/update_service_spec.rb
@@ -12,7 +12,7 @@
 
       expect(result[:status]).to eq(:success)
       expect(result[:pin]).to match(/^\d{6}$/)
-      expect(result[:expires_at]).to be_within(1.minute).of(24.hours.from_now)
+      expect(result[:expires_at]).to be_within(1.minute).of(described_class::SUPPORT_PIN_EXPIRATION)
     end
 
     it 'stores the PIN in Redis' do
-- 
GitLab


From 6d645764f391157e58082609826efed72c083299 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Wed, 18 Dec 2024 13:25:14 +1000
Subject: [PATCH 07/11] Use strong_memoize for support_pin_data

Changed the instance variable memoization to use strong_memoize instead,
as per MR discussion.
---
 app/models/user.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index fc182378ed8956..2796934a5f8a5e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2561,7 +2561,9 @@ def add_admin_note(new_note)
   
   # rubocop: disable CodeReuse/ServiceClass
   def support_pin_data
-    @support_pin_data ||= Users::SupportPin::RetrieveService.new(self).execute
+    strong_memoize(:support_pin_data) do
+      Users::SupportPin::RetrieveService.new(self).execute
+    end
   end
   # rubocop: enable CodeReuse/ServiceClass
 
-- 
GitLab


From fc8d502249f23f4b862461f6db56850082b83c3b Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Wed, 18 Dec 2024 13:28:21 +1000
Subject: [PATCH 08/11] Fix Support PIN GET API response message

A temporary testing response was committed unintentionally. This commit fixes the message
to be more appropriate, as per MR discussion.
---
 lib/api/users.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/api/users.rb b/lib/api/users.rb
index aefdb2bba2ad98..fb63981707d5cc 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -118,7 +118,7 @@ def reorder_users(users)
         begin
           result = ::Users::SupportPin::RetrieveService.new(user).execute
         rescue StandardError
-          error!("lol", :unprocessable_entity)
+          error!("Error retrieving Support PIN for user.", :unprocessable_entity)
         end
 
         if result
-- 
GitLab


From 5ea0b9a4ae513b164c9b99c398175177966de8e5 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Wed, 18 Dec 2024 13:34:31 +1000
Subject: [PATCH 09/11] Changes to rspec tests

- Change let to let_it_be in some cases
- Fix failing tests after adding strong_memoize for support_pin_data
- Add tests reported by the undercoverage job
---
 spec/models/user_spec.rb                      | 21 ++++++++++---------
 spec/requests/api/users_spec.rb               | 18 ++++++++++++++++
 .../users/support_pin/update_service_spec.rb  |  8 +++++++
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 08aa88e76aaefe..cef37ca28e17e3 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -9183,52 +9183,53 @@ def owner_class_attribute_default; end
   end
 
   describe 'support pin methods' do
-    let(:user) { create(:user) }
+    let_it_be(:user_with_pin) { create(:user) }
+    let_it_be(:user_no_pin) { create(:user) }
     let(:pin_data) { { pin: '123456', expires_at: 7.days.from_now } }
     let(:retrieve_service) { instance_double(Users::SupportPin::RetrieveService) }
 
     before do
-      allow(Users::SupportPin::RetrieveService).to receive(:new).with(user).and_return(retrieve_service)
+      allow(Users::SupportPin::RetrieveService).to receive(:new).and_return(retrieve_service)
     end
 
     describe '#support_pin' do
       it 'returns the pin when it exists' do
         allow(retrieve_service).to receive(:execute).and_return(pin_data)
-        expect(user.support_pin).to eq('123456')
+        expect(user_with_pin.support_pin).to eq('123456')
       end
 
       it 'returns nil when no pin exists' do
         allow(retrieve_service).to receive(:execute).and_return(nil)
-        expect(user.support_pin).to be_nil
+        expect(user_no_pin.support_pin).to be_nil
       end
 
       it 'returns nil when pin key is missing' do
         allow(retrieve_service).to receive(:execute).and_return({})
-        expect(user.support_pin).to be_nil
+        expect(user_no_pin.support_pin).to be_nil
       end
     end
 
     describe '#support_pin_expires_at' do
       it 'returns the expiration time when it exists' do
         allow(retrieve_service).to receive(:execute).and_return(pin_data)
-        expect(user.support_pin_expires_at).to eq(pin_data[:expires_at])
+        expect(user_with_pin.support_pin_expires_at).to be_within(1.second).of(pin_data[:expires_at])
       end
 
       it 'returns nil when no expiration time exists' do
         allow(retrieve_service).to receive(:execute).and_return(nil)
-        expect(user.support_pin_expires_at).to be_nil
+        expect(user_no_pin.support_pin_expires_at).to be_nil
       end
 
       it 'returns nil when expires_at key is missing' do
         allow(retrieve_service).to receive(:execute).and_return({})
-        expect(user.support_pin_expires_at).to be_nil
+        expect(user_no_pin.support_pin_expires_at).to be_nil
       end
     end
 
     it 'only calls the retrieve service once for multiple method calls' do
       expect(retrieve_service).to receive(:execute).once.and_return(pin_data)
-      user.support_pin
-      user.support_pin_expires_at
+      user_with_pin.support_pin
+      user_with_pin.support_pin_expires_at
     end
   end
 end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 120f3d52dea05c..a704d35ba4aab3 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -5401,6 +5401,15 @@ def update_password(user, admin, password = User.random_password)
         expect(response).to have_gitlab_http_status(:created)
         expect(json_response).to include('pin', 'expires_at')
       end
+
+      it "handles errors when creating a support PIN" do
+        allow_next_instance_of(Users::SupportPin::UpdateService) do |instance|
+          allow(instance).to receive(:execute).and_return({ status: :error, message: "Failed to create support PIN" })
+        end
+        post api("/user/support_pin", user)
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect(json_response["error"]).to eq("Failed to create support PIN")
+      end
     end
 
     context 'when not authenticated' do
@@ -5443,6 +5452,15 @@ def update_password(user, admin, password = User.random_password)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      it "handles errors when retrieving the support PIN" do
+        allow_next_instance_of(Users::SupportPin::RetrieveService) do |instance|
+          allow(instance).to receive(:execute).and_raise(StandardError)
+        end
+        get api("/users/#{user.id}/support_pin", admin, admin_mode: true)
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect(json_response["error"]).to eq("Error retrieving Support PIN for user.")
+      end
     end
 
     context 'when authenticated as non-admin' do
diff --git a/spec/services/users/support_pin/update_service_spec.rb b/spec/services/users/support_pin/update_service_spec.rb
index bd67f897fe705e..e0e19990533d4d 100644
--- a/spec/services/users/support_pin/update_service_spec.rb
+++ b/spec/services/users/support_pin/update_service_spec.rb
@@ -24,5 +24,13 @@
         expect(redis.get(key)).to eq(pin)
       end
     end
+
+    it "returns an error when storing the PIN fails" do
+      allow_next_instance_of(described_class) do |instance|
+        allow(instance).to receive(:store_pin).and_return(false)
+      end
+      result = described_class.new(user).execute
+      expect(result).to eq({ status: :error, message: 'Failed to create support PIN' })
+    end
   end
 end
-- 
GitLab


From f6af94a6890aeee60a8b5feee477e1a26f55ca37 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Thu, 19 Dec 2024 11:49:33 +1000
Subject: [PATCH 10/11] Apply suggestion to separate expect statement in rspec
 test

---
 spec/models/user_spec.rb | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index cef37ca28e17e3..f11dc7d7c734f3 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -9195,16 +9195,19 @@ def owner_class_attribute_default; end
     describe '#support_pin' do
       it 'returns the pin when it exists' do
         allow(retrieve_service).to receive(:execute).and_return(pin_data)
+
         expect(user_with_pin.support_pin).to eq('123456')
       end
 
       it 'returns nil when no pin exists' do
         allow(retrieve_service).to receive(:execute).and_return(nil)
+
         expect(user_no_pin.support_pin).to be_nil
       end
 
       it 'returns nil when pin key is missing' do
         allow(retrieve_service).to receive(:execute).and_return({})
+
         expect(user_no_pin.support_pin).to be_nil
       end
     end
@@ -9212,22 +9215,26 @@ def owner_class_attribute_default; end
     describe '#support_pin_expires_at' do
       it 'returns the expiration time when it exists' do
         allow(retrieve_service).to receive(:execute).and_return(pin_data)
+
         expect(user_with_pin.support_pin_expires_at).to be_within(1.second).of(pin_data[:expires_at])
       end
 
       it 'returns nil when no expiration time exists' do
         allow(retrieve_service).to receive(:execute).and_return(nil)
+
         expect(user_no_pin.support_pin_expires_at).to be_nil
       end
 
       it 'returns nil when expires_at key is missing' do
         allow(retrieve_service).to receive(:execute).and_return({})
+
         expect(user_no_pin.support_pin_expires_at).to be_nil
       end
     end
 
     it 'only calls the retrieve service once for multiple method calls' do
       expect(retrieve_service).to receive(:execute).once.and_return(pin_data)
+
       user_with_pin.support_pin
       user_with_pin.support_pin_expires_at
     end
-- 
GitLab


From bb7f8f8cd8b52b3c127d0ab5fe164bb926e57303 Mon Sep 17 00:00:00 2001
From: Michael Trainor <mtrainor@gitlab.com>
Date: Thu, 19 Dec 2024 14:23:08 +1000
Subject: [PATCH 11/11] Fix rubocop offence for trailing whitespace

---
 app/models/user.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 2796934a5f8a5e..8a9f667bfc7f90 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2558,7 +2558,7 @@ def uploads_sharding_key
   def add_admin_note(new_note)
     self.note = "#{new_note}\n#{self.note}"
   end
-  
+
   # rubocop: disable CodeReuse/ServiceClass
   def support_pin_data
     strong_memoize(:support_pin_data) do
-- 
GitLab