diff --git a/app/controllers/user_settings/personal_access_tokens_controller.rb b/app/controllers/user_settings/personal_access_tokens_controller.rb index de1a33647fa38bf262d796ef82dab7a3096cabb1..d0e7a189535797d1cb31ad2a47a6fa7869e4f325 100644 --- a/app/controllers/user_settings/personal_access_tokens_controller.rb +++ b/app/controllers/user_settings/personal_access_tokens_controller.rb @@ -6,8 +6,10 @@ class PersonalAccessTokensController < ApplicationController include FeedTokenHelper feature_category :system_access - before_action :check_personal_access_tokens_enabled + before_action do + push_frontend_feature_flag :advanced_token_scopes, current_user + end prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:ics) } def index @@ -67,7 +69,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 4135b398b09827f675d2e49f3fc36d64ca0cbf60..af37e80e9f0dde89844b1749022b0b223c119948 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 @@ -48,6 +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 validate :expires_at_before_instance_max_expiry_date, on: :create def revoke! @@ -87,6 +89,47 @@ def validate_scopes end end + def validate_advanced_scopes + return unless advanced_scopes + + lines = advanced_scopes.each_line + validate_max_lines(lines) + + return if errors[:advanced_scopes].any? + + lines.each_with_index do |line, index| + validate_advanced_scope_line(line, index + 1) + end + 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) + end + end + + def validate_advanced_scope_line(line, line_number) + verb, path, rest = line.split(" ") + + if rest.present? || verb.blank? || path.blank? + errors.add :advanced_scopes, format(_("%{line} must contain two space-separated values"), line: line) + return + end + + begin + 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/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb index eb2e66a9285384ee39654a4fc4c3d73c4b9b2e23..3b8591bdaa5e7c3a16b2b006748bf1c4dc289536 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,15 @@ def include_any_scope?(required_scopes) end end end + + def validate_advanced_scopes! + 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) + match_verb.match(request.request_method) && match_path.match(request.path) + end + + INSUFFICIENT_SCOPE + end end diff --git a/app/services/personal_access_tokens/create_service.rb b/app/services/personal_access_tokens/create_service.rb index 095cfadf02c80bf491ea3a9abf4a3022ed0ce7e4..a70d1821009c5cf9e1364bbdd9948a71938139b0 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 0000000000000000000000000000000000000000..9c42f51cb472e66791e5a6dc68e2ebcc1ef410fe --- /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 b9f831021a182b51c4d3987cad7f5f4709aa3c93..b589895202979bc0ccf247a87f2f237003a48fdd 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/locale/gitlab.pot b/locale/gitlab.pot index 23e83b2a9fe47068651e6c1723ffd05121db676e..5f79d0f21f3178c505c3e419a2fa1c3106166503 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 "" 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 3636c970e8389f531afc512e7546a89cb8e18eec..b5902964d9bd04f2f53b71e4e652cccfe86fbb42 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 } diff --git a/spec/lib/api/entities/personal_access_token_spec.rb b/spec/lib/api/entities/personal_access_token_spec.rb index 039b55022313eb92470ff5cb652cc1f3e0ad0108..4a23ef9d89af06b7261562a6c99605ee3a34bb60 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,6 +21,7 @@ user_id: user.id, last_used_at: nil, active: true, + advanced_scopes: "POST /some_path\nPUT /another_path", expires_at: token.expires_at.iso8601 }) end diff --git a/spec/lib/gitlab/auth/auth_finders_spec.rb b/spec/lib/gitlab/auth/auth_finders_spec.rb index f96906cad01d3a1992ef2ccc082ece16be86417f..89a37cca8f4d245cc68f1c66006ed7db4b6a12cd 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_and_save_access_token! }.to raise_error(Gitlab::Auth::InsufficientScopeError) + end end context 'with impersonation token' do diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 384bf40b0be85826b5c51e6a054c4b5fe2ab0158..6b78075438c2bafa4a1d0dac2a1bde3bae70340a 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -293,6 +293,57 @@ end end + 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 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 #{PersonalAccessToken::ADVANCED_SCOPES_MAX_LINES}.") + end + end + + 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" } + + context 'with valid scopes' do + 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]).to be_empty + end + end + + 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_line_format} must contain two space-separated values") + end + + 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 '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 + expect(personal_access_token.errors[:advanced_scopes]).to contain_exactly(/error in path regex at line 1: .*/) + end + end + end + describe 'scopes' do describe '.active' do let_it_be(:revoked_token) { create(:personal_access_token, :revoked) } diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb index 4cdce094358ff89aab986744b88888af277a4cac..1f081038e48127f026b66fbe3001fc0dcb7899de 100644 --- a/spec/services/access_token_validation_service_spec.rb +++ b/spec/services/access_token_validation_service_spec.rb @@ -71,4 +71,72 @@ 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 + + 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 the correct validation result" do + expect(validation_result).to eq(expected_result) + end + end + + context 'when there is a matching advanced scope' do + 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 + 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 + 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 + 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 + 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