From 4b9c680acef49adbfc854e6386b87a2d68bd2a70 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 24 May 2024 09:52:14 -0400
Subject: [PATCH 01/16] Add backend changes for MVC on advanced token scopes

Changelog: added
---
 .../personal_access_tokens_controller.rb        |  9 +++++++--
 app/models/personal_access_token.rb             | 16 ++++++++++++++++
 app/services/access_token_validation_service.rb | 17 +++++++++++++++++
 .../personal_access_tokens/create_service.rb    |  3 ++-
 .../feature_flags/wip/advanced_token_scopes.yml |  9 +++++++++
 lib/api/entities/personal_access_token.rb       |  2 ++
 .../api/entities/personal_access_token_spec.rb  |  1 +
 7 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 config/feature_flags/wip/advanced_token_scopes.yml

diff --git a/app/controllers/user_settings/personal_access_tokens_controller.rb b/app/controllers/user_settings/personal_access_tokens_controller.rb
index de1a33647fa38b..352a9bf6068617 100644
--- a/app/controllers/user_settings/personal_access_tokens_controller.rb
+++ b/app/controllers/user_settings/personal_access_tokens_controller.rb
@@ -6,10 +6,13 @@ class PersonalAccessTokensController < ApplicationController
     include FeedTokenHelper
 
     feature_category :system_access
-
     before_action :check_personal_access_tokens_enabled
     prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:ics) }
 
+    before_action do
+      push_frontend_feature_flag :advanced_token_scopes, current_user
+    end
+
     def index
       set_index_vars
       scopes = params[:scopes].split(',').map(&:squish).select(&:present?).map(&:to_sym) unless params[:scopes].nil?
@@ -67,7 +70,9 @@ def finder(options = {})
     end
 
     def personal_access_token_params
-      params.require(:personal_access_token).permit(:name, :expires_at, scopes: [])
+      permitted_params = [:name, :expires_at, { scopes: [] }]
+      permitted_params << :advanced_scopes if ::Feature.enabled?(:advanced_token_scopes, current_user)
+      params.require(:personal_access_token).permit(*permitted_params)
     end
 
     def set_index_vars
diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 4135b398b09827..58b0d1d6b12370 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -48,6 +48,7 @@ class PersonalAccessToken < ApplicationRecord
   validates :expires_at, presence: true, on: :create, unless: :allow_expires_at_to_be_empty?
 
   validate :validate_scopes
+  validate :validate_advanced_scopes, if: -> { advanced_scopes.present? }
   validate :expires_at_before_instance_max_expiry_date, on: :create
 
   def revoke!
@@ -87,6 +88,21 @@ def validate_scopes
     end
   end
 
+  def validate_advanced_scopes
+    return unless Feature.enabled?(:advanced_token_scopes, user)
+
+    advanced_scopes.each_line do |line|
+      scope = line.split(" ")
+      errors.add :advanced_scopes, "invalid format" unless scope.length == 2
+      begin
+        Gitlab::UntrustedRegexp.new(scope[0])
+        Gitlab::UntrustedRegexp.new(scope[1])
+      rescue RegexpError => e
+        errors.add :advanced_scopes, e.message
+      end
+    end
+  end
+
   def set_default_scopes
     # When only loading a select set of attributes, for example using `EachBatch`,
     # the `scopes` attribute is not present, so we can't initialize it.
diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index eb2e66a9285384..3deae0ff6af0c8 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -22,6 +22,9 @@ def validate(scopes: [])
     elsif token.revoked?
       REVOKED
 
+    elsif token.is_a?(PersonalAccessToken) && token.advanced_scopes?
+      validate_advanced_scopes!
+
     elsif !self.include_any_scope?(scopes)
       INSUFFICIENT_SCOPE
 
@@ -52,4 +55,18 @@ def include_any_scope?(required_scopes)
       end
     end
   end
+
+  def validate_advanced_scopes!
+    result = INSUFFICIENT_SCOPE
+    token.advanced_scopes.each_line do |scope|
+      verb, path = scope.split(" ")
+      match_verb = Gitlab::UntrustedRegexp.new(verb)
+      match_path = Gitlab::UntrustedRegexp.new(path)
+      if match_verb.match(request.request_method) && match_path.match(request.path)
+        result = VALID
+        break
+      end
+    end
+    result
+  end
 end
diff --git a/app/services/personal_access_tokens/create_service.rb b/app/services/personal_access_tokens/create_service.rb
index 095cfadf02c80b..a70d1821009c5c 100644
--- a/app/services/personal_access_tokens/create_service.rb
+++ b/app/services/personal_access_tokens/create_service.rb
@@ -36,7 +36,8 @@ def personal_access_token_params
         name: params[:name],
         impersonation: params[:impersonation] || false,
         scopes: params[:scopes],
-        expires_at: pat_expiration
+        expires_at: pat_expiration,
+        advanced_scopes: params[:advanced_scopes]
       }
     end
 
diff --git a/config/feature_flags/wip/advanced_token_scopes.yml b/config/feature_flags/wip/advanced_token_scopes.yml
new file mode 100644
index 00000000000000..9c42f51cb472e6
--- /dev/null
+++ b/config/feature_flags/wip/advanced_token_scopes.yml
@@ -0,0 +1,9 @@
+---
+name: advanced_token_scopes
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/368904
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/150045
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/457013
+milestone: '17.1'
+group: group::authentication
+type: wip
+default_enabled: false
diff --git a/lib/api/entities/personal_access_token.rb b/lib/api/entities/personal_access_token.rb
index b9f831021a182b..b589895202979b 100644
--- a/lib/api/entities/personal_access_token.rb
+++ b/lib/api/entities/personal_access_token.rb
@@ -11,6 +11,8 @@ class PersonalAccessToken < Grape::Entity
       expose :user_id, documentation: { type: 'integer', example: 3 }
       expose :last_used_at, documentation: { type: 'dateTime', example: '2020-08-31T15:53:00.073Z' }
       expose :active?, as: :active, documentation: { type: 'boolean' }
+      expose :advanced_scopes,
+        documentation: { type: 'string', example: '^GET$ ^/api/v4/user$\n^POST$ ^/api/v4/issues$' }
       expose :expires_at, documentation:
         { type: 'dateTime', example: '2020-08-31T15:53:00.073Z' } do |personal_access_token|
         personal_access_token.expires_at ? personal_access_token.expires_at.iso8601 : nil
diff --git a/spec/lib/api/entities/personal_access_token_spec.rb b/spec/lib/api/entities/personal_access_token_spec.rb
index 039b55022313eb..1f745d34aaf378 100644
--- a/spec/lib/api/entities/personal_access_token_spec.rb
+++ b/spec/lib/api/entities/personal_access_token_spec.rb
@@ -19,6 +19,7 @@
          user_id: user.id,
          last_used_at: nil,
          active: true,
+         advanced_scopes: nil,
          expires_at: token.expires_at.iso8601
        })
     end
-- 
GitLab


From 45a7295b3a8237152fa41251785c21ec3faf9fc5 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 24 May 2024 10:24:33 -0400
Subject: [PATCH 02/16] Introduce limit of 10 lines of scope per token

---
 app/models/personal_access_token.rb | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 58b0d1d6b12370..90bc77267f4c04 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -91,7 +91,13 @@ def validate_scopes
   def validate_advanced_scopes
     return unless Feature.enabled?(:advanced_token_scopes, user)
 
-    advanced_scopes.each_line do |line|
+    lines = advanced_scopes.each_line.to_a
+    if lines.size > 10
+      errors.add :advanced_scopes, "too many lines"
+      return
+    end
+
+    lines.each do |line|
       scope = line.split(" ")
       errors.add :advanced_scopes, "invalid format" unless scope.length == 2
       begin
-- 
GitLab


From c2d80748facd75c4905b0bbe37b03897458b36cb Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 24 May 2024 11:41:07 -0400
Subject: [PATCH 03/16] Add model validation spec

---
 app/models/personal_access_token.rb       |  2 +
 spec/models/personal_access_token_spec.rb | 53 +++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 90bc77267f4c04..7464658561afc3 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -100,6 +100,8 @@ def validate_advanced_scopes
     lines.each do |line|
       scope = line.split(" ")
       errors.add :advanced_scopes, "invalid format" unless scope.length == 2
+      break unless scope.length == 2
+
       begin
         Gitlab::UntrustedRegexp.new(scope[0])
         Gitlab::UntrustedRegexp.new(scope[1])
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 384bf40b0be858..e9ba5ddc5a3198 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -255,6 +255,18 @@
       end
     end
 
+    context 'when advanced_token_scopes is enabled' do
+      before do
+        stub_feature_flags(advanced_token_scopes: true)
+      end
+
+      it "allows creating a token with advanced_scopes" do
+        personal_access_token.advanced_scopes = "^GET$|^POST$ ^/api/v4/projects/9/issues$"
+
+        expect(personal_access_token).to be_valid
+      end
+    end
+
     it "rejects creating a token with unavailable scopes" do
       personal_access_token.scopes = [:openid, :api]
 
@@ -293,6 +305,47 @@
     end
   end
 
+  describe '#validate_advanced_scopes' do
+    let(:user) { create(:user) }
+    let(:personal_access_token) { build(:personal_access_token) }
+
+    context 'when advanced_scopes has more than 10 lines' do
+      let(:advanced_scopes) { "line1\n" * 11 }
+
+      it 'adds a "too many lines" error' do
+        personal_access_token.advanced_scopes = advanced_scopes
+        personal_access_token.send(:validate_advanced_scopes)
+        expect(personal_access_token.errors[:advanced_scopes]).to include('too many lines')
+      end
+    end
+
+    context 'when advanced_scopes has 10 lines or fewer' do
+      let(:valid_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" * 10 }
+      let(:invalid_line_format) { "invalid_scope\n" }
+      let(:invalid_regex_format) { "this_is_not_a_regex this_is_not_a_regex\n" }
+
+      it 'does not add a "too many lines" error' do
+        personal_access_token.advanced_scopes = valid_scopes
+        personal_access_token.send(:validate_advanced_scopes)
+        expect(personal_access_token.errors[:advanced_scopes]).not_to include('too many lines')
+      end
+
+      it 'validates the format of each line' do
+        personal_access_token.advanced_scopes = invalid_line_format
+        personal_access_token.send(:validate_advanced_scopes)
+        expect(personal_access_token.errors[:advanced_scopes]).to include('invalid format')
+      end
+
+      it 'validates each scope with Gitlab::UntrustedRegexp' do
+        personal_access_token.advanced_scopes = invalid_regex_format
+        allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError, 'invalid regex')
+
+        personal_access_token.send(:validate_advanced_scopes)
+        expect(personal_access_token.errors[:advanced_scopes]).to include('invalid regex')
+      end
+    end
+  end
+
   describe 'scopes' do
     describe '.active' do
       let_it_be(:revoked_token) { create(:personal_access_token, :revoked) }
-- 
GitLab


From bda92d6a53f17785ecd95c37d23b1243d2079092 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 24 May 2024 11:46:38 -0400
Subject: [PATCH 04/16] Improve spec

---
 spec/models/personal_access_token_spec.rb | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index e9ba5ddc5a3198..c95454ad91fb1b 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -324,10 +324,14 @@
       let(:invalid_line_format) { "invalid_scope\n" }
       let(:invalid_regex_format) { "this_is_not_a_regex this_is_not_a_regex\n" }
 
-      it 'does not add a "too many lines" error' do
-        personal_access_token.advanced_scopes = valid_scopes
-        personal_access_token.send(:validate_advanced_scopes)
-        expect(personal_access_token.errors[:advanced_scopes]).not_to include('too many lines')
+      context 'with valid scopes' do
+        it 'does not add return any errors' do
+          personal_access_token.advanced_scopes = valid_scopes
+          personal_access_token.send(:validate_advanced_scopes)
+          expect(personal_access_token.errors[:advanced_scopes]).not_to include('too many lines')
+          expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid format')
+          expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid regex')
+        end
       end
 
       it 'validates the format of each line' do
-- 
GitLab


From 1af739440dd8a2bb4673620289012a27cf5808d1 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 24 May 2024 14:17:57 -0400
Subject: [PATCH 05/16] Add validation service spec for advanced scopes

---
 spec/models/personal_access_token_spec.rb     | 14 +--
 .../access_token_validation_service_spec.rb   | 87 +++++++++++++++++++
 2 files changed, 88 insertions(+), 13 deletions(-)

diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index c95454ad91fb1b..d2bede612d6d92 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -255,18 +255,6 @@
       end
     end
 
-    context 'when advanced_token_scopes is enabled' do
-      before do
-        stub_feature_flags(advanced_token_scopes: true)
-      end
-
-      it "allows creating a token with advanced_scopes" do
-        personal_access_token.advanced_scopes = "^GET$|^POST$ ^/api/v4/projects/9/issues$"
-
-        expect(personal_access_token).to be_valid
-      end
-    end
-
     it "rejects creating a token with unavailable scopes" do
       personal_access_token.scopes = [:openid, :api]
 
@@ -325,7 +313,7 @@
       let(:invalid_regex_format) { "this_is_not_a_regex this_is_not_a_regex\n" }
 
       context 'with valid scopes' do
-        it 'does not add return any errors' do
+        it 'does not return any errors' do
           personal_access_token.advanced_scopes = valid_scopes
           personal_access_token.send(:validate_advanced_scopes)
           expect(personal_access_token.errors[:advanced_scopes]).not_to include('too many lines')
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 4cdce094358ff8..02bdae67816d26 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -71,4 +71,91 @@
       end
     end
   end
+
+  describe '#validate_advanced_scopes!' do
+    let(:request) { double('request') }
+    let(:token) { double('token', advanced_scopes: advanced_scopes) }
+    let(:service) { described_class.new(token, request: request) }
+
+    subject(:validation_result) { service.validate_advanced_scopes! }
+
+    before do
+      allow(Gitlab::UntrustedRegexp).to receive(:new).and_call_original
+    end
+
+    context 'when there is a matching advanced scope' do
+      let(:advanced_scopes) { "GET /some_path\nPOST /another_path" }
+      let(:request_method) { 'GET' }
+      let(:request_path) { '/some_path' }
+
+      before do
+        allow(request).to receive(:request_method).and_return(request_method)
+        allow(request).to receive(:path).and_return(request_path)
+      end
+
+      it 'returns VALID' do
+        expect(validation_result).to eq(:valid)
+      end
+    end
+
+    context 'when there is no matching advanced scope' do
+      let(:advanced_scopes) { "POST /some_path\nPUT /another_path" }
+      let(:request_method) { 'GET' }
+      let(:request_path) { '/some_path' }
+
+      before do
+        allow(request).to receive(:request_method).and_return(request_method)
+        allow(request).to receive(:path).and_return(request_path)
+      end
+
+      it 'returns INSUFFICIENT_SCOPE' do
+        expect(validation_result).to eq(:insufficient_scope)
+      end
+    end
+
+    context 'when scopes are empty' do
+      let(:advanced_scopes) { "" }
+      let(:request_method) { 'GET' }
+      let(:request_path) { '/some_path' }
+
+      before do
+        allow(request).to receive(:request_method).and_return(request_method)
+        allow(request).to receive(:path).and_return(request_path)
+      end
+
+      it 'returns INSUFFICIENT_SCOPE' do
+        expect(validation_result).to eq(:insufficient_scope)
+      end
+    end
+
+    context 'when request method and path do not match any scope' do
+      let(:advanced_scopes) { "POST /some_path\nPUT /another_path" }
+      let(:request_method) { 'DELETE' }
+      let(:request_path) { '/non_matching_path' }
+
+      before do
+        allow(request).to receive(:request_method).and_return(request_method)
+        allow(request).to receive(:path).and_return(request_path)
+      end
+
+      it 'returns INSUFFICIENT_SCOPE' do
+        expect(validation_result).to eq(:insufficient_scope)
+      end
+    end
+
+    context 'when multiple scopes are provided and one matches' do
+      let(:advanced_scopes) { "GET /some_path\nPOST /another_path" }
+      let(:request_method) { 'POST' }
+      let(:request_path) { '/another_path' }
+
+      before do
+        allow(request).to receive(:request_method).and_return(request_method)
+        allow(request).to receive(:path).and_return(request_path)
+      end
+
+      it 'returns VALID' do
+        expect(validation_result).to eq(:valid)
+      end
+    end
+  end
 end
-- 
GitLab


From 831701b697188607eb791eaf58781f6eabd50232 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Mon, 27 May 2024 15:02:46 -0400
Subject: [PATCH 06/16] Add validation logic for turned off FF

---
 app/models/personal_access_token.rb       |  5 ++++-
 spec/models/personal_access_token_spec.rb | 22 ++++++++++++++++++----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 7464658561afc3..060c582761663c 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -89,7 +89,10 @@ def validate_scopes
   end
 
   def validate_advanced_scopes
-    return unless Feature.enabled?(:advanced_token_scopes, user)
+    unless Feature.enabled?(:advanced_token_scopes, user)
+      errors.add :advanced_scopes, "is not allowed"
+      return
+    end
 
     lines = advanced_scopes.each_line.to_a
     if lines.size > 10
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index d2bede612d6d92..1a01c5010c609e 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -302,7 +302,7 @@
 
       it 'adds a "too many lines" error' do
         personal_access_token.advanced_scopes = advanced_scopes
-        personal_access_token.send(:validate_advanced_scopes)
+        personal_access_token.validate
         expect(personal_access_token.errors[:advanced_scopes]).to include('too many lines')
       end
     end
@@ -315,7 +315,7 @@
       context 'with valid scopes' do
         it 'does not return any errors' do
           personal_access_token.advanced_scopes = valid_scopes
-          personal_access_token.send(:validate_advanced_scopes)
+          personal_access_token.validate
           expect(personal_access_token.errors[:advanced_scopes]).not_to include('too many lines')
           expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid format')
           expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid regex')
@@ -324,7 +324,7 @@
 
       it 'validates the format of each line' do
         personal_access_token.advanced_scopes = invalid_line_format
-        personal_access_token.send(:validate_advanced_scopes)
+        personal_access_token.validate
         expect(personal_access_token.errors[:advanced_scopes]).to include('invalid format')
       end
 
@@ -332,10 +332,24 @@
         personal_access_token.advanced_scopes = invalid_regex_format
         allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError, 'invalid regex')
 
-        personal_access_token.send(:validate_advanced_scopes)
+        personal_access_token.validate
         expect(personal_access_token.errors[:advanced_scopes]).to include('invalid regex')
       end
     end
+
+    context 'when advanced_token_scopes feature flag is turned off' do
+      before do
+        stub_feature_flags(advanced_token_scopes: false)
+      end
+
+      let(:advanced_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" }
+
+      it 'adds an "is not allowed" error' do
+        personal_access_token.advanced_scopes = advanced_scopes
+        personal_access_token.validate
+        expect(personal_access_token.errors[:advanced_scopes]).to include('is not allowed')
+      end
+    end
   end
 
   describe 'scopes' do
-- 
GitLab


From 8c16887c46def5d497289521798beb75bd2ae54a Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Wed, 29 May 2024 11:41:35 -0400
Subject: [PATCH 07/16] Fix second round of review comments

---
 .../personal_access_tokens_controller.rb      |  5 +-
 app/models/personal_access_token.rb           | 57 +++++++++++++------
 locale/gitlab.pot                             |  9 +++
 spec/models/personal_access_token_spec.rb     | 19 +++----
 .../access_token_validation_service_spec.rb   | 10 ++--
 5 files changed, 63 insertions(+), 37 deletions(-)

diff --git a/app/controllers/user_settings/personal_access_tokens_controller.rb b/app/controllers/user_settings/personal_access_tokens_controller.rb
index 352a9bf6068617..14b05272166c24 100644
--- a/app/controllers/user_settings/personal_access_tokens_controller.rb
+++ b/app/controllers/user_settings/personal_access_tokens_controller.rb
@@ -7,12 +7,9 @@ class PersonalAccessTokensController < ApplicationController
 
     feature_category :system_access
     before_action :check_personal_access_tokens_enabled
+    before_action push_frontend_feature_flag :advanced_token_scopes, current_user
     prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:ics) }
 
-    before_action do
-      push_frontend_feature_flag :advanced_token_scopes, current_user
-    end
-
     def index
       set_index_vars
       scopes = params[:scopes].split(',').map(&:squish).select(&:present?).map(&:to_sym) unless params[:scopes].nil?
diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 060c582761663c..04b7729e579b6f 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -89,28 +89,49 @@ def validate_scopes
   end
 
   def validate_advanced_scopes
-    unless Feature.enabled?(:advanced_token_scopes, user)
-      errors.add :advanced_scopes, "is not allowed"
-      return
+    return if error_added_for_not_allowed?
+
+    lines = advanced_scopes.each_line
+    return if error_added_for_too_many_lines?(lines)
+
+    lines.each do |line|
+      validate_advanced_scope_line(line)
     end
+  end
 
-    lines = advanced_scopes.each_line.to_a
-    if lines.size > 10
-      errors.add :advanced_scopes, "too many lines"
-      return
+  def error_added_for_not_allowed?
+    return false if Feature.enabled?(:advanced_token_scopes, user)
+
+    errors.add :advanced_scopes, _("is not allowed")
+    true
+  end
+
+  def error_added_for_too_many_lines?(lines)
+    max_size = 10
+    if lines.count > max_size
+      errors.add :advanced_scopes, format(_("is too long %{size}. The maximum size is %{max_size}."), size: lines.count, max_size: max_size)
+      return true
     end
 
-    lines.each do |line|
-      scope = line.split(" ")
-      errors.add :advanced_scopes, "invalid format" unless scope.length == 2
-      break unless scope.length == 2
-
-      begin
-        Gitlab::UntrustedRegexp.new(scope[0])
-        Gitlab::UntrustedRegexp.new(scope[1])
-      rescue RegexpError => e
-        errors.add :advanced_scopes, e.message
-      end
+    false
+  end
+
+  def error_added_for_invalid_format?(scope)
+    return false if scope.length == 2
+
+    errors.add :advanced_scopes, _("line must contain two space-separated values")
+    true
+  end
+
+  def validate_advanced_scope_line(line)
+    scope = line.split(" ")
+    return if error_added_for_invalid_format?(scope)
+
+    begin
+      Gitlab::UntrustedRegexp.new(scope[0])
+      Gitlab::UntrustedRegexp.new(scope[1])
+    rescue RegexpError => e
+      errors.add :advanced_scopes, e.message
     end
   end
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 23e83b2a9fe470..23f53b403face7 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -62439,6 +62439,9 @@ msgstr ""
 msgid "is not a valid X509 certificate."
 msgstr ""
 
+msgid "is not allowed"
+msgstr ""
+
 msgid "is not allowed for sign-up. Please use your regular email address."
 msgstr ""
 
@@ -62481,6 +62484,9 @@ msgstr ""
 msgid "is read-only"
 msgstr ""
 
+msgid "is too long %{size}. The maximum size is %{max_size}."
+msgstr ""
+
 msgid "is too long (%{current_value}). The maximum size is %{max_size}."
 msgstr ""
 
@@ -62569,6 +62575,9 @@ msgid_plural "lines"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "line must contain two space-separated values"
+msgstr ""
+
 msgid "load it anyway"
 msgstr ""
 
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 1a01c5010c609e..441b339462c4c1 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -293,22 +293,23 @@
     end
   end
 
-  describe '#validate_advanced_scopes' do
+  describe 'validation on advanced_scopes' do
     let(:user) { create(:user) }
     let(:personal_access_token) { build(:personal_access_token) }
+    let(:valid_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" * 10 }
 
     context 'when advanced_scopes has more than 10 lines' do
-      let(:advanced_scopes) { "line1\n" * 11 }
+      let(:too_long_size) { 11 }
+      let(:advanced_scopes) { "line1\n" * too_long_size }
 
-      it 'adds a "too many lines" error' do
+      it 'adds a "is too long" error' do
         personal_access_token.advanced_scopes = advanced_scopes
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include('too many lines')
+        expect(personal_access_token.errors[:advanced_scopes]).to include(/is too long #{too_long_size}. The maximum size is .*/)
       end
     end
 
     context 'when advanced_scopes has 10 lines or fewer' do
-      let(:valid_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" * 10 }
       let(:invalid_line_format) { "invalid_scope\n" }
       let(:invalid_regex_format) { "this_is_not_a_regex this_is_not_a_regex\n" }
 
@@ -316,7 +317,7 @@
         it 'does not return any errors' do
           personal_access_token.advanced_scopes = valid_scopes
           personal_access_token.validate
-          expect(personal_access_token.errors[:advanced_scopes]).not_to include('too many lines')
+          expect(personal_access_token.errors[:advanced_scopes]).not_to include('is too long')
           expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid format')
           expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid regex')
         end
@@ -325,7 +326,7 @@
       it 'validates the format of each line' do
         personal_access_token.advanced_scopes = invalid_line_format
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include('invalid format')
+        expect(personal_access_token.errors[:advanced_scopes]).to include('line must contain two space-separated values')
       end
 
       it 'validates each scope with Gitlab::UntrustedRegexp' do
@@ -342,10 +343,8 @@
         stub_feature_flags(advanced_token_scopes: false)
       end
 
-      let(:advanced_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" }
-
       it 'adds an "is not allowed" error' do
-        personal_access_token.advanced_scopes = advanced_scopes
+        personal_access_token.advanced_scopes = valid_scopes
         personal_access_token.validate
         expect(personal_access_token.errors[:advanced_scopes]).to include('is not allowed')
       end
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 02bdae67816d26..1d9d5c2acef32c 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -93,7 +93,7 @@
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it 'returns VALID' do
+      it 'returns :valid' do
         expect(validation_result).to eq(:valid)
       end
     end
@@ -108,7 +108,7 @@
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it 'returns INSUFFICIENT_SCOPE' do
+      it 'returns :insufficient_scope' do
         expect(validation_result).to eq(:insufficient_scope)
       end
     end
@@ -123,7 +123,7 @@
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it 'returns INSUFFICIENT_SCOPE' do
+      it 'returns :insufficient_scope' do
         expect(validation_result).to eq(:insufficient_scope)
       end
     end
@@ -138,7 +138,7 @@
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it 'returns INSUFFICIENT_SCOPE' do
+      it 'returns :insufficient_scope' do
         expect(validation_result).to eq(:insufficient_scope)
       end
     end
@@ -153,7 +153,7 @@
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it 'returns VALID' do
+      it 'returns :valid' do
         expect(validation_result).to eq(:valid)
       end
     end
-- 
GitLab


From 60a66672d9160544b645d40d1202ec345e52263b Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Wed, 29 May 2024 13:37:32 -0400
Subject: [PATCH 08/16] Reintroduce before do to fix undefined method

---
 .../user_settings/personal_access_tokens_controller.rb        | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/controllers/user_settings/personal_access_tokens_controller.rb b/app/controllers/user_settings/personal_access_tokens_controller.rb
index 14b05272166c24..d0e7a189535797 100644
--- a/app/controllers/user_settings/personal_access_tokens_controller.rb
+++ b/app/controllers/user_settings/personal_access_tokens_controller.rb
@@ -7,7 +7,9 @@ class PersonalAccessTokensController < ApplicationController
 
     feature_category :system_access
     before_action :check_personal_access_tokens_enabled
-    before_action push_frontend_feature_flag :advanced_token_scopes, current_user
+    before_action do
+      push_frontend_feature_flag :advanced_token_scopes, current_user
+    end
     prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:ics) }
 
     def index
-- 
GitLab


From 56624cac4e41f2d83f99d9745b6d9b5df6a9cf47 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 31 May 2024 15:42:42 -0400
Subject: [PATCH 09/16] Address comments of round 2

---
 app/models/personal_access_token.rb           | 37 +++++-----
 locale/gitlab.pot                             | 11 ++-
 spec/models/personal_access_token_spec.rb     | 25 ++++---
 .../access_token_validation_service_spec.rb   | 70 +++++--------------
 4 files changed, 56 insertions(+), 87 deletions(-)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 04b7729e579b6f..9a335b7ee857c9 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -16,6 +16,7 @@ class PersonalAccessToken < ApplicationRecord
   # PATs are 20 characters + optional configurable settings prefix (0..20)
   TOKEN_LENGTH_RANGE = (20..40)
   MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS = 365
+  ADVANCED_SCOPES_MAX_LINES = 10
 
   serialize :scopes, Array # rubocop:disable Cop/ActiveRecordSerialize
 
@@ -94,47 +95,49 @@ def validate_advanced_scopes
     lines = advanced_scopes.each_line
     return if error_added_for_too_many_lines?(lines)
 
-    lines.each do |line|
-      validate_advanced_scope_line(line)
+    lines.each_with_index do |line, index|
+      validate_advanced_scope_line(line, index + 1)
     end
   end
 
   def error_added_for_not_allowed?
     return false if Feature.enabled?(:advanced_token_scopes, user)
 
-    errors.add :advanced_scopes, _("is not allowed")
+    errors.add :advanced_scopes, _("Action not allowed.")
     true
   end
 
   def error_added_for_too_many_lines?(lines)
-    max_size = 10
-    if lines.count > max_size
-      errors.add :advanced_scopes, format(_("is too long %{size}. The maximum size is %{max_size}."), size: lines.count, max_size: max_size)
+    if lines.count > ADVANCED_SCOPES_MAX_LINES
+      errors.add :advanced_scopes, format(_("is too long %{current_value}. The maximum size is %{max_size}."), current_value: lines.count, max_size: ADVANCED_SCOPES_MAX_LINES)
       return true
     end
 
     false
   end
 
-  def error_added_for_invalid_format?(scope)
-    return false if scope.length == 2
+  def validate_advanced_scope_line(line, line_number)
+    verb, path, rest = line.split(" ")
 
-    errors.add :advanced_scopes, _("line must contain two space-separated values")
-    true
-  end
-
-  def validate_advanced_scope_line(line)
-    scope = line.split(" ")
-    return if error_added_for_invalid_format?(scope)
+    if rest.present? || verb.blank? || path.blank?
+      errors.add :advanced_scopes, format(_("%{line} must contain two space-separated values"), line: line)
+      return
+    end
 
     begin
-      Gitlab::UntrustedRegexp.new(scope[0])
-      Gitlab::UntrustedRegexp.new(scope[1])
+      validate_regex(verb, "verb", line_number)
+      validate_regex(path, "path", line_number)
     rescue RegexpError => e
       errors.add :advanced_scopes, e.message
     end
   end
 
+  def validate_regex(pattern, part, line_number)
+    Gitlab::UntrustedRegexp.new(pattern)
+  rescue RegexpError => e
+    raise RegexpError, "error in #{part} regex at line #{line_number}: #{e.message}"
+  end
+
   def set_default_scopes
     # When only loading a select set of attributes, for example using `EachBatch`,
     # the `scopes` attribute is not present, so we can't initialize it.
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 23f53b403face7..9fd4e358810439 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -974,6 +974,9 @@ msgstr ""
 msgid "%{level_name} is not allowed since the fork source project has lower visibility."
 msgstr ""
 
+msgid "%{line} must contain two space-separated values"
+msgstr ""
+
 msgid "%{linkStart} Learn more%{linkEnd}."
 msgstr ""
 
@@ -62439,9 +62442,6 @@ msgstr ""
 msgid "is not a valid X509 certificate."
 msgstr ""
 
-msgid "is not allowed"
-msgstr ""
-
 msgid "is not allowed for sign-up. Please use your regular email address."
 msgstr ""
 
@@ -62484,7 +62484,7 @@ msgstr ""
 msgid "is read-only"
 msgstr ""
 
-msgid "is too long %{size}. The maximum size is %{max_size}."
+msgid "is too long %{current_value}. The maximum size is %{max_size}."
 msgstr ""
 
 msgid "is too long (%{current_value}). The maximum size is %{max_size}."
@@ -62575,9 +62575,6 @@ msgid_plural "lines"
 msgstr[0] ""
 msgstr[1] ""
 
-msgid "line must contain two space-separated values"
-msgstr ""
-
 msgid "load it anyway"
 msgstr ""
 
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 441b339462c4c1..104e8aa08e9dbf 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -299,19 +299,20 @@
     let(:valid_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" * 10 }
 
     context 'when advanced_scopes has more than 10 lines' do
-      let(:too_long_size) { 11 }
+      let(:too_long_size) { PersonalAccessToken::ADVANCED_SCOPES_MAX_LINES + 1 }
       let(:advanced_scopes) { "line1\n" * too_long_size }
 
       it 'adds a "is too long" error' do
         personal_access_token.advanced_scopes = advanced_scopes
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include(/is too long #{too_long_size}. The maximum size is .*/)
+        expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/is too long #{too_long_size}. The maximum size is .*/)
       end
     end
 
     context 'when advanced_scopes has 10 lines or fewer' do
       let(:invalid_line_format) { "invalid_scope\n" }
-      let(:invalid_regex_format) { "this_is_not_a_regex this_is_not_a_regex\n" }
+      let(:invalid_verb_regex_format) { "[abc ^/api/v4/projects/9/issues$" }
+      let(:invalid_path_regex_format) { "^/api/v4/projects/9/issues$ [abc" }
 
       context 'with valid scopes' do
         it 'does not return any errors' do
@@ -326,15 +327,21 @@
       it 'validates the format of each line' do
         personal_access_token.advanced_scopes = invalid_line_format
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include('line must contain two space-separated values')
+        expect(personal_access_token.errors[:advanced_scopes]).to include("#{invalid_line_format} must contain two space-separated values")
       end
 
-      it 'validates each scope with Gitlab::UntrustedRegexp' do
-        personal_access_token.advanced_scopes = invalid_regex_format
-        allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError, 'invalid regex')
+      it 'raises an error when the verb part of a regex is not valid' do
+        personal_access_token.advanced_scopes = invalid_verb_regex_format
 
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include('invalid regex')
+        expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/error in verb regex at line 1: .*/)
+      end
+
+      it 'raises an error when the path part of a regex is not valid' do
+        personal_access_token.advanced_scopes = invalid_path_regex_format
+
+        personal_access_token.validate
+        expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/error in path regex at line 1: .*/)
       end
     end
 
@@ -346,7 +353,7 @@
       it 'adds an "is not allowed" error' do
         personal_access_token.advanced_scopes = valid_scopes
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include('is not allowed')
+        expect(personal_access_token.errors[:advanced_scopes]).to include('Action not allowed.')
       end
     end
   end
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 1d9d5c2acef32c..15d84f9fb0457a 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -83,79 +83,41 @@
       allow(Gitlab::UntrustedRegexp).to receive(:new).and_call_original
     end
 
-    context 'when there is a matching advanced scope' do
-      let(:advanced_scopes) { "GET /some_path\nPOST /another_path" }
-      let(:request_method) { 'GET' }
-      let(:request_path) { '/some_path' }
+    shared_examples 'a scope validation' do |scopes, method, path, expected_result|
+      let(:advanced_scopes) { scopes }
+      let(:request_method) { method }
+      let(:request_path) { path }
 
       before do
         allow(request).to receive(:request_method).and_return(request_method)
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it 'returns :valid' do
-        expect(validation_result).to eq(:valid)
+      it "returns #{expected_result}" do
+        expect(validation_result).to eq(expected_result)
       end
     end
 
-    context 'when there is no matching advanced scope' do
-      let(:advanced_scopes) { "POST /some_path\nPUT /another_path" }
-      let(:request_method) { 'GET' }
-      let(:request_path) { '/some_path' }
-
-      before do
-        allow(request).to receive(:request_method).and_return(request_method)
-        allow(request).to receive(:path).and_return(request_path)
-      end
+    context 'when there is a matching advanced scope' do
+      include_examples 'a scope validation', "GET /some_path", 'GET', '/some_path', :valid
+    end
 
-      it 'returns :insufficient_scope' do
-        expect(validation_result).to eq(:insufficient_scope)
-      end
+    context 'when there is no matching advanced scope' do
+      include_examples 'a scope validation', "POST /some_path\nPUT /another_path", 'GET', '/some_path',
+        :insufficient_scope
     end
 
     context 'when scopes are empty' do
-      let(:advanced_scopes) { "" }
-      let(:request_method) { 'GET' }
-      let(:request_path) { '/some_path' }
-
-      before do
-        allow(request).to receive(:request_method).and_return(request_method)
-        allow(request).to receive(:path).and_return(request_path)
-      end
-
-      it 'returns :insufficient_scope' do
-        expect(validation_result).to eq(:insufficient_scope)
-      end
+      include_examples 'a scope validation', '', 'GET', '/some_path', :insufficient_scope
     end
 
     context 'when request method and path do not match any scope' do
-      let(:advanced_scopes) { "POST /some_path\nPUT /another_path" }
-      let(:request_method) { 'DELETE' }
-      let(:request_path) { '/non_matching_path' }
-
-      before do
-        allow(request).to receive(:request_method).and_return(request_method)
-        allow(request).to receive(:path).and_return(request_path)
-      end
-
-      it 'returns :insufficient_scope' do
-        expect(validation_result).to eq(:insufficient_scope)
-      end
+      include_examples 'a scope validation', "POST /some_path\nPUT /another_path", 'DELETE', '/non_matching_path',
+        :insufficient_scope
     end
 
     context 'when multiple scopes are provided and one matches' do
-      let(:advanced_scopes) { "GET /some_path\nPOST /another_path" }
-      let(:request_method) { 'POST' }
-      let(:request_path) { '/another_path' }
-
-      before do
-        allow(request).to receive(:request_method).and_return(request_method)
-        allow(request).to receive(:path).and_return(request_path)
-      end
-
-      it 'returns :valid' do
-        expect(validation_result).to eq(:valid)
-      end
+      include_examples 'a scope validation', "GET /some_path\nPOST /another_path", 'POST', '/another_path', :valid
     end
   end
 end
-- 
GitLab


From c6a2c2aabe3bf611bee545b1a8aa00610b5000e6 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Wed, 5 Jun 2024 16:34:24 -0400
Subject: [PATCH 10/16] Address round 3 of comments

---
 app/models/personal_access_token.rb           |  2 +-
 locale/gitlab.pot                             |  3 --
 spec/models/personal_access_token_spec.rb     | 16 +++----
 .../access_token_validation_service_spec.rb   | 45 +++++++++++++------
 4 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 9a335b7ee857c9..3f86069c9f9b19 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -109,7 +109,7 @@ def error_added_for_not_allowed?
 
   def error_added_for_too_many_lines?(lines)
     if lines.count > ADVANCED_SCOPES_MAX_LINES
-      errors.add :advanced_scopes, format(_("is too long %{current_value}. The maximum size is %{max_size}."), current_value: lines.count, max_size: ADVANCED_SCOPES_MAX_LINES)
+      errors.add :advanced_scopes, format(_("is too long (%{current_value}). The maximum size is %{max_size}."), current_value: lines.count, max_size: ADVANCED_SCOPES_MAX_LINES)
       return true
     end
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 9fd4e358810439..5f79d0f21f3178 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -62484,9 +62484,6 @@ msgstr ""
 msgid "is read-only"
 msgstr ""
 
-msgid "is too long %{current_value}. The maximum size is %{max_size}."
-msgstr ""
-
 msgid "is too long (%{current_value}). The maximum size is %{max_size}."
 msgstr ""
 
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 104e8aa08e9dbf..a5d32922ba03a7 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -298,18 +298,18 @@
     let(:personal_access_token) { build(:personal_access_token) }
     let(:valid_scopes) { "^GET|POST$ ^/api/v4/projects/9/issues$\n" * 10 }
 
-    context 'when advanced_scopes has more than 10 lines' do
+    context 'when advanced_scopes has more lines than allowed' do
       let(:too_long_size) { PersonalAccessToken::ADVANCED_SCOPES_MAX_LINES + 1 }
       let(:advanced_scopes) { "line1\n" * too_long_size }
 
       it 'adds a "is too long" error' do
         personal_access_token.advanced_scopes = advanced_scopes
         personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/is too long #{too_long_size}. The maximum size is .*/)
+        expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly("is too long (#{too_long_size}). The maximum size is #{PersonalAccessToken::ADVANCED_SCOPES_MAX_LINES}.")
       end
     end
 
-    context 'when advanced_scopes has 10 lines or fewer' do
+    context 'when advanced_scopes does not exceed the line limit' do
       let(:invalid_line_format) { "invalid_scope\n" }
       let(:invalid_verb_regex_format) { "[abc ^/api/v4/projects/9/issues$" }
       let(:invalid_path_regex_format) { "^/api/v4/projects/9/issues$ [abc" }
@@ -318,9 +318,7 @@
         it 'does not return any errors' do
           personal_access_token.advanced_scopes = valid_scopes
           personal_access_token.validate
-          expect(personal_access_token.errors[:advanced_scopes]).not_to include('is too long')
-          expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid format')
-          expect(personal_access_token.errors[:advanced_scopes]).not_to include('invalid regex')
+          expect(personal_access_token.errors[:advanced_scopes]).to be_empty
         end
       end
 
@@ -330,14 +328,14 @@
         expect(personal_access_token.errors[:advanced_scopes]).to include("#{invalid_line_format} must contain two space-separated values")
       end
 
-      it 'raises an error when the verb part of a regex is not valid' do
+      it 'rejects creating a token when the verb part of its regex is not valid' do
         personal_access_token.advanced_scopes = invalid_verb_regex_format
 
         personal_access_token.validate
         expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/error in verb regex at line 1: .*/)
       end
 
-      it 'raises an error when the path part of a regex is not valid' do
+      it 'rejects creating a token when the path part of its regex is not valid' do
         personal_access_token.advanced_scopes = invalid_path_regex_format
 
         personal_access_token.validate
@@ -350,7 +348,7 @@
         stub_feature_flags(advanced_token_scopes: false)
       end
 
-      it 'adds an "is not allowed" error' do
+      it 'adds an "Action not allowed" error' do
         personal_access_token.advanced_scopes = valid_scopes
         personal_access_token.validate
         expect(personal_access_token.errors[:advanced_scopes]).to include('Action not allowed.')
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 15d84f9fb0457a..1f081038e48127 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -83,41 +83,60 @@
       allow(Gitlab::UntrustedRegexp).to receive(:new).and_call_original
     end
 
-    shared_examples 'a scope validation' do |scopes, method, path, expected_result|
-      let(:advanced_scopes) { scopes }
-      let(:request_method) { method }
-      let(:request_path) { path }
-
+    shared_examples 'returning the result of validation' do
       before do
         allow(request).to receive(:request_method).and_return(request_method)
         allow(request).to receive(:path).and_return(request_path)
       end
 
-      it "returns #{expected_result}" do
+      it "returns the correct validation result" do
         expect(validation_result).to eq(expected_result)
       end
     end
 
     context 'when there is a matching advanced scope' do
-      include_examples 'a scope validation', "GET /some_path", 'GET', '/some_path', :valid
+      let(:advanced_scopes) { "GET /some_path" }
+      let(:request_method) { 'GET' }
+      let(:request_path) { '/some_path' }
+      let(:expected_result) { :valid }
+
+      include_examples 'returning the result of validation'
     end
 
     context 'when there is no matching advanced scope' do
-      include_examples 'a scope validation', "POST /some_path\nPUT /another_path", 'GET', '/some_path',
-        :insufficient_scope
+      let(:advanced_scopes) { "POST /some_path\nPUT /another_path" }
+      let(:request_method) { 'GET' }
+      let(:request_path) { '/some_path' }
+      let(:expected_result) { :insufficient_scope }
+
+      include_examples 'returning the result of validation'
     end
 
     context 'when scopes are empty' do
-      include_examples 'a scope validation', '', 'GET', '/some_path', :insufficient_scope
+      let(:advanced_scopes) { '' }
+      let(:request_method) { 'GET' }
+      let(:request_path) { '/some_path' }
+      let(:expected_result) { :insufficient_scope }
+
+      include_examples 'returning the result of validation'
     end
 
     context 'when request method and path do not match any scope' do
-      include_examples 'a scope validation', "POST /some_path\nPUT /another_path", 'DELETE', '/non_matching_path',
-        :insufficient_scope
+      let(:advanced_scopes) { "POST /some_path\nPUT /another_path" }
+      let(:request_method) { 'DELETE' }
+      let(:request_path) { '/non_matching_path' }
+      let(:expected_result) { :insufficient_scope }
+
+      include_examples 'returning the result of validation'
     end
 
     context 'when multiple scopes are provided and one matches' do
-      include_examples 'a scope validation', "GET /some_path\nPOST /another_path", 'POST', '/another_path', :valid
+      let(:advanced_scopes) { "GET /some_path\nPOST /another_path" }
+      let(:request_method) { 'POST' }
+      let(:request_path) { '/another_path' }
+      let(:expected_result) { :valid }
+
+      include_examples 'returning the result of validation'
     end
   end
 end
-- 
GitLab


From d0a0351b076e68537f824263a7ecb88377e4c5ed Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 7 Jun 2024 09:48:54 -0400
Subject: [PATCH 11/16] Add round 4 of comments

---
 app/models/personal_access_token.rb | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 3f86069c9f9b19..c8539e4c7e7b1f 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -49,7 +49,7 @@ class PersonalAccessToken < ApplicationRecord
   validates :expires_at, presence: true, on: :create, unless: :allow_expires_at_to_be_empty?
 
   validate :validate_scopes
-  validate :validate_advanced_scopes, if: -> { advanced_scopes.present? }
+  validate :validate_advanced_scopes
   validate :expires_at_before_instance_max_expiry_date, on: :create
 
   def revoke!
@@ -90,10 +90,14 @@ def validate_scopes
   end
 
   def validate_advanced_scopes
+    return unless advanced_scopes
+
     return if error_added_for_not_allowed?
 
     lines = advanced_scopes.each_line
-    return if error_added_for_too_many_lines?(lines)
+    validate_max_lines(lines)
+
+    return if errors[:advanced_scopes].any?
 
     lines.each_with_index do |line, index|
       validate_advanced_scope_line(line, index + 1)
@@ -107,13 +111,10 @@ def error_added_for_not_allowed?
     true
   end
 
-  def error_added_for_too_many_lines?(lines)
+  def validate_max_lines(lines)
     if lines.count > ADVANCED_SCOPES_MAX_LINES
       errors.add :advanced_scopes, format(_("is too long (%{current_value}). The maximum size is %{max_size}."), current_value: lines.count, max_size: ADVANCED_SCOPES_MAX_LINES)
-      return true
     end
-
-    false
   end
 
   def validate_advanced_scope_line(line, line_number)
-- 
GitLab


From 423536774edd244e23d351601384b7e0a1a449b4 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 7 Jun 2024 10:41:34 -0400
Subject: [PATCH 12/16] Addressing more comments

---
 app/models/personal_access_token.rb                 |  9 ---------
 app/services/access_token_validation_service.rb     | 11 ++++-------
 spec/lib/api/entities/personal_access_token_spec.rb |  6 ++++--
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index c8539e4c7e7b1f..af37e80e9f0dde 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -92,8 +92,6 @@ def validate_scopes
   def validate_advanced_scopes
     return unless advanced_scopes
 
-    return if error_added_for_not_allowed?
-
     lines = advanced_scopes.each_line
     validate_max_lines(lines)
 
@@ -104,13 +102,6 @@ def validate_advanced_scopes
     end
   end
 
-  def error_added_for_not_allowed?
-    return false if Feature.enabled?(:advanced_token_scopes, user)
-
-    errors.add :advanced_scopes, _("Action not allowed.")
-    true
-  end
-
   def validate_max_lines(lines)
     if lines.count > ADVANCED_SCOPES_MAX_LINES
       errors.add :advanced_scopes, format(_("is too long (%{current_value}). The maximum size is %{max_size}."), current_value: lines.count, max_size: ADVANCED_SCOPES_MAX_LINES)
diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index 3deae0ff6af0c8..3b8591bdaa5e7c 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -57,16 +57,13 @@ def include_any_scope?(required_scopes)
   end
 
   def validate_advanced_scopes!
-    result = INSUFFICIENT_SCOPE
-    token.advanced_scopes.each_line do |scope|
+    return VALID if token.advanced_scopes.each_line.any? do |scope|
       verb, path = scope.split(" ")
       match_verb = Gitlab::UntrustedRegexp.new(verb)
       match_path = Gitlab::UntrustedRegexp.new(path)
-      if match_verb.match(request.request_method) && match_path.match(request.path)
-        result = VALID
-        break
-      end
+      match_verb.match(request.request_method) && match_path.match(request.path)
     end
-    result
+
+    INSUFFICIENT_SCOPE
   end
 end
diff --git a/spec/lib/api/entities/personal_access_token_spec.rb b/spec/lib/api/entities/personal_access_token_spec.rb
index 1f745d34aaf378..4a23ef9d89af06 100644
--- a/spec/lib/api/entities/personal_access_token_spec.rb
+++ b/spec/lib/api/entities/personal_access_token_spec.rb
@@ -5,7 +5,9 @@
 RSpec.describe API::Entities::PersonalAccessToken do
   describe '#as_json' do
     let_it_be(:user) { create(:user) }
-    let_it_be(:token) { create(:personal_access_token, user: user) }
+    let_it_be(:token) do
+      create(:personal_access_token, user: user, advanced_scopes: "POST /some_path\nPUT /another_path")
+    end
 
     let(:entity) { described_class.new(token) }
 
@@ -19,7 +21,7 @@
          user_id: user.id,
          last_used_at: nil,
          active: true,
-         advanced_scopes: nil,
+         advanced_scopes: "POST /some_path\nPUT /another_path",
          expires_at: token.expires_at.iso8601
        })
     end
-- 
GitLab


From 7830cc2df48076aba7280dcadefe5ea7f40d85b6 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 7 Jun 2024 11:23:44 -0400
Subject: [PATCH 13/16] Add advanced_scopes to resource_access_token schema

---
 .../public_api/v4/resource_access_token.json  | 55 +++++++++++++++----
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/spec/fixtures/api/schemas/public_api/v4/resource_access_token.json b/spec/fixtures/api/schemas/public_api/v4/resource_access_token.json
index 3636c970e8389f..b5902964d9bd04 100644
--- a/spec/fixtures/api/schemas/public_api/v4/resource_access_token.json
+++ b/spec/fixtures/api/schemas/public_api/v4/resource_access_token.json
@@ -13,19 +13,54 @@
     "last_used_at"
   ],
   "properties": {
-    "id": { "type": "integer" },
-    "name": { "type": "string" },
-    "user_id": { "type": "integer" },
-    "active": { "type": "boolean" },
-    "created_at": { "type": "string", "format": "date-time" },
-    "expires_at": { "type": ["string", "null"], "format": "date" },
-    "revoked": { "type": "boolean" },
-    "access_level": { "type": "integer" },
+    "id": {
+      "type": "integer"
+    },
+    "name": {
+      "type": "string"
+    },
+    "user_id": {
+      "type": "integer"
+    },
+    "active": {
+      "type": "boolean"
+    },
+    "created_at": {
+      "type": "string",
+      "format": "date-time"
+    },
+    "expires_at": {
+      "type": [
+        "string",
+        "null"
+      ],
+      "format": "date"
+    },
+    "revoked": {
+      "type": "boolean"
+    },
+    "access_level": {
+      "type": "integer"
+    },
+    "advanced_scopes": {
+      "type": [
+        "string",
+        "null"
+      ]
+    },
     "scopes": {
       "type": "array",
-      "items": { "type": "string" }
+      "items": {
+        "type": "string"
+      }
     },
-    "last_used_at": { "type": ["string", "null"], "format": "date-time" }
+    "last_used_at": {
+      "type": [
+        "string",
+        "null"
+      ],
+      "format": "date-time"
+    }
   },
   "additionalProperties": false
 }
-- 
GitLab


From 15cb98a98e469b8ac88713bfde62b76b8699bd75 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 7 Jun 2024 13:37:26 -0400
Subject: [PATCH 14/16] Remove test associated to feature flag on model

---
 spec/models/personal_access_token_spec.rb | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index a5d32922ba03a7..6b78075438c2ba 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -342,18 +342,6 @@
         expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/error in path regex at line 1: .*/)
       end
     end
-
-    context 'when advanced_token_scopes feature flag is turned off' do
-      before do
-        stub_feature_flags(advanced_token_scopes: false)
-      end
-
-      it 'adds an "Action not allowed" error' do
-        personal_access_token.advanced_scopes = valid_scopes
-        personal_access_token.validate
-        expect(personal_access_token.errors[:advanced_scopes]).to include('Action not allowed.')
-      end
-    end
   end
 
   describe 'scopes' do
-- 
GitLab


From 672d1aca01dc24e4fa87e81a9917ca9dce94daa5 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Fri, 7 Jun 2024 15:31:37 -0400
Subject: [PATCH 15/16] Add test to auth finders

---
 spec/lib/gitlab/auth/auth_finders_spec.rb | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/spec/lib/gitlab/auth/auth_finders_spec.rb b/spec/lib/gitlab/auth/auth_finders_spec.rb
index f96906cad01d3a..63997b6e7dd34d 100644
--- a/spec/lib/gitlab/auth/auth_finders_spec.rb
+++ b/spec/lib/gitlab/auth/auth_finders_spec.rb
@@ -971,6 +971,7 @@ def auth_header_with(token)
 
       before do
         allow_any_instance_of(described_class).to receive(:access_token).and_return(personal_access_token)
+        allow_any_instance_of(AccessTokenValidationService).to receive(:request).and_return(double('request', request_method: 'GET', path: '/some_path'))
       end
 
       it 'saves the token info in the environment' do
@@ -999,6 +1000,12 @@ def auth_header_with(token)
           expect(request.env).not_to have_key(described_class::API_TOKEN_ENV)
         end
       end
+
+      it 'returns Gitlab::Auth::InsufficientScopeError if invalid advanced token scopes' do
+        personal_access_token.update!(advanced_scopes: "INVALID_VERB /INVALID_PATH")
+
+        expect { validate_access_token! }.to raise_error(Gitlab::Auth::InsufficientScopeError)
+      end
     end
 
     context 'with impersonation token' do
-- 
GitLab


From a4724b4b62291de1f46b98c2b834e58c54a5cbb9 Mon Sep 17 00:00:00 2001
From: unset <fveillette@gitlab.com>
Date: Mon, 10 Jun 2024 15:06:47 -0400
Subject: [PATCH 16/16] Rebase and change method name

---
 spec/lib/gitlab/auth/auth_finders_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/auth/auth_finders_spec.rb b/spec/lib/gitlab/auth/auth_finders_spec.rb
index 63997b6e7dd34d..89a37cca8f4d24 100644
--- a/spec/lib/gitlab/auth/auth_finders_spec.rb
+++ b/spec/lib/gitlab/auth/auth_finders_spec.rb
@@ -1004,7 +1004,7 @@ def auth_header_with(token)
       it 'returns Gitlab::Auth::InsufficientScopeError if invalid advanced token scopes' do
         personal_access_token.update!(advanced_scopes: "INVALID_VERB /INVALID_PATH")
 
-        expect { validate_access_token! }.to raise_error(Gitlab::Auth::InsufficientScopeError)
+        expect { validate_and_save_access_token! }.to raise_error(Gitlab::Auth::InsufficientScopeError)
       end
     end
 
-- 
GitLab