From 798ecf9f02d5c2a2c28216f7a4dd7c060a1107f7 Mon Sep 17 00:00:00 2001 From: "parag.jain" Date: Mon, 2 Dec 2024 20:13:17 +0530 Subject: [PATCH 1/4] skip rotation for read Apis --- .../api/connect/base_controller.rb | 8 ++++++++ .../connect/v3/systems/products_controller.rb | 1 + .../connect/v3/systems/systems_controller.rb | 1 + .../connect/v4/systems/products_controller.rb | 1 + app/controllers/application_controller.rb | 6 +++--- .../api/connect/base_controller_spec.rb | 19 +++---------------- 6 files changed, 17 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/connect/base_controller.rb b/app/controllers/api/connect/base_controller.rb index 1fbeaff07..1983fb267 100644 --- a/app/controllers/api/connect/base_controller.rb +++ b/app/controllers/api/connect/base_controller.rb @@ -44,4 +44,12 @@ def authenticate_with_token end end + def system_token_header + headers[SYSTEM_TOKEN_HEADER] = @system.system_token + end + + def refresh_system_token + @system.update(system_token: SecureRandom.uuid) + system_token_header + end end diff --git a/app/controllers/api/connect/v3/systems/products_controller.rb b/app/controllers/api/connect/v3/systems/products_controller.rb index 2545a9e92..fcad5de58 100644 --- a/app/controllers/api/connect/v3/systems/products_controller.rb +++ b/app/controllers/api/connect/v3/systems/products_controller.rb @@ -5,6 +5,7 @@ class Api::Connect::V3::Systems::ProductsController < Api::Connect::BaseControll before_action :check_product_service_and_repositories, only: %i[show activate] before_action :load_subscription, only: %i[activate upgrade] before_action :check_base_product_dependencies, only: %i[activate upgrade show] + after_action :refresh_system_token, only: %i[activate upgrade], if: -> { request.headers.key?(SYSTEM_TOKEN_HEADER) } def activate create_product_activation diff --git a/app/controllers/api/connect/v3/systems/systems_controller.rb b/app/controllers/api/connect/v3/systems/systems_controller.rb index 863557279..d9a17154d 100644 --- a/app/controllers/api/connect/v3/systems/systems_controller.rb +++ b/app/controllers/api/connect/v3/systems/systems_controller.rb @@ -1,6 +1,7 @@ class Api::Connect::V3::Systems::SystemsController < Api::Connect::BaseController before_action :authenticate_system + after_action :refresh_system_token, only: [:update], if: -> { request.headers.key?(SYSTEM_TOKEN_HEADER) } def update if params[:online_at].present? diff --git a/app/controllers/api/connect/v4/systems/products_controller.rb b/app/controllers/api/connect/v4/systems/products_controller.rb index 7237e5a80..c92df74d5 100644 --- a/app/controllers/api/connect/v4/systems/products_controller.rb +++ b/app/controllers/api/connect/v4/systems/products_controller.rb @@ -1,5 +1,6 @@ class Api::Connect::V4::Systems::ProductsController < Api::Connect::V3::Systems::ProductsController + after_action :refresh_system_token, only: %i[synchronize destroy], if: -> { request.headers.key?(SYSTEM_TOKEN_HEADER) } def destroy if @product.base? raise ActionController::TranslatedError.new(N_('The product "%s" is a base product and cannot be deactivated'), @product.name) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 686972895..ba11c3a75 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -21,11 +21,11 @@ def authenticate_system(skip_on_duplicated: false) update_user_agent # If SYSTEM_TOKEN_HEADER is present, RMT assumes the client uses a SUSEConnect version - # that supports this feature. In this case, refresh the token and include it in the response. + # that supports this feature. if system_tokens_enabled? && request.headers.key?(SYSTEM_TOKEN_HEADER) - @system.update(last_seen_at: Time.zone.now, system_token: SecureRandom.uuid) + @system.update(last_seen_at: Time.zone.now) headers[SYSTEM_TOKEN_HEADER] = @system.system_token - # only update last_seen_at each 3 minutes, + # only update last_seen_at each 3 minutes, # so that a system that calls SCC every second doesn't write + lock the database row elsif !@system.last_seen_at || @system.last_seen_at < 3.minutes.ago @system.touch(:last_seen_at) diff --git a/spec/requests/api/connect/base_controller_spec.rb b/spec/requests/api/connect/base_controller_spec.rb index 70ef9aaff..76b1f4b6b 100644 --- a/spec/requests/api/connect/base_controller_spec.rb +++ b/spec/requests/api/connect/base_controller_spec.rb @@ -55,15 +55,6 @@ def require_product end end - shared_examples 'updates the system token' do - it 'updates the system token' do - allow(SecureRandom).to receive(:uuid).and_return(new_system_token) - - expect { get :service, params: { id: 1 } } - .to change { system.reload.system_token } - .from(current_system_token).to(new_system_token) - end - end shared_examples "does not update the old system's token" do it 'does not update the system token' do @@ -74,7 +65,6 @@ def require_product shared_examples 'creates a duplicate system' do it 'creates a new System (duplicate)' do - allow(SecureRandom).to receive(:uuid).and_return(new_system_token) expect { get :service, params: { id: 1 } } .to change { System.get_by_credentials(system.login, system.password).count } @@ -85,7 +75,6 @@ def require_product expect(duplicate_system).not_to eq(system) expect(duplicate_system.activations.count).to eq(system.activations.count) expect(duplicate_system.system_token).not_to eq(system.system_token) - expect(duplicate_system.system_token).to eq(new_system_token) end end @@ -182,8 +171,7 @@ def require_product let(:system) { create(:system, hostname: 'system') } include_examples 'does not create a duplicate system' - include_examples 'updates the system token' - include_examples 'responds with a new token' + include_examples "does not update the old system's token" end context 'when the system has a token and the header matches it' do @@ -193,8 +181,8 @@ def require_product let(:system) { create(:system, hostname: 'system', system_token: current_system_token) } include_examples 'does not create a duplicate system' - include_examples 'updates the system token' - include_examples 'responds with a new token' + include_examples "does not update the old system's token" + end context 'when the system has a token and the header is blank' do @@ -208,7 +196,6 @@ def require_product include_examples "does not update the old system's token" include_examples 'creates a duplicate system' - include_examples 'responds with a new token' end context 'when the system has a token and the header does not match it' do From 3f5a6e8d3d8fcf2b8ff7503e26a37652718642dd Mon Sep 17 00:00:00 2001 From: Thomas Schmidt Date: Wed, 4 Dec 2024 00:09:08 +0100 Subject: [PATCH 2/4] fix rubocop --- app/controllers/application_controller.rb | 2 +- spec/requests/api/connect/base_controller_spec.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ba11c3a75..bb7504888 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -25,7 +25,7 @@ def authenticate_system(skip_on_duplicated: false) if system_tokens_enabled? && request.headers.key?(SYSTEM_TOKEN_HEADER) @system.update(last_seen_at: Time.zone.now) headers[SYSTEM_TOKEN_HEADER] = @system.system_token - # only update last_seen_at each 3 minutes, + # only update last_seen_at each 3 minutes, # so that a system that calls SCC every second doesn't write + lock the database row elsif !@system.last_seen_at || @system.last_seen_at < 3.minutes.ago @system.touch(:last_seen_at) diff --git a/spec/requests/api/connect/base_controller_spec.rb b/spec/requests/api/connect/base_controller_spec.rb index 76b1f4b6b..8832909ea 100644 --- a/spec/requests/api/connect/base_controller_spec.rb +++ b/spec/requests/api/connect/base_controller_spec.rb @@ -65,7 +65,6 @@ def require_product shared_examples 'creates a duplicate system' do it 'creates a new System (duplicate)' do - expect { get :service, params: { id: 1 } } .to change { System.get_by_credentials(system.login, system.password).count } .by(1) @@ -182,7 +181,6 @@ def require_product include_examples 'does not create a duplicate system' include_examples "does not update the old system's token" - end context 'when the system has a token and the header is blank' do From 59131b6353fd1b2c29a17a4f158e34306dbfcf8f Mon Sep 17 00:00:00 2001 From: "parag.jain" Date: Wed, 4 Dec 2024 12:22:50 +0530 Subject: [PATCH 3/4] add testcases supporting token rotation for write api; add testcases for header token addition in read api --- app/controllers/application_controller.rb | 2 +- .../v3/systems/activations_controller_spec.rb | 23 ++++++ .../v3/systems/products_controller_spec.rb | 55 ++++++++++++++ .../v3/systems/systems_controller_spec.rb | 19 +++++ .../v4/systems/products_controller_spec.rb | 73 +++++++++++++++++++ 5 files changed, 171 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index bb7504888..63458615a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -24,7 +24,7 @@ def authenticate_system(skip_on_duplicated: false) # that supports this feature. if system_tokens_enabled? && request.headers.key?(SYSTEM_TOKEN_HEADER) @system.update(last_seen_at: Time.zone.now) - headers[SYSTEM_TOKEN_HEADER] = @system.system_token + system_token_header # only update last_seen_at each 3 minutes, # so that a system that calls SCC every second doesn't write + lock the database row elsif !@system.last_seen_at || @system.last_seen_at < 3.minutes.ago diff --git a/spec/requests/api/connect/v3/systems/activations_controller_spec.rb b/spec/requests/api/connect/v3/systems/activations_controller_spec.rb index a4ff0a562..6a62ee2bd 100644 --- a/spec/requests/api/connect/v3/systems/activations_controller_spec.rb +++ b/spec/requests/api/connect/v3/systems/activations_controller_spec.rb @@ -69,5 +69,28 @@ expect(system.scc_synced_at).to be_nil end end + + context 'system token header' do + context 'when system token header is present in request' do + let(:token_headers) do + authenticated_headers.merge({ 'System-Token' => 'some_token' }) + end + + it 'sets system token in response headers' do + get url, headers: token_headers + expect(response.code).to eq '200' + expect(response.headers).to include('System-Token') + expect(response.headers['System-Token']).not_to be_nil + expect(response.headers['System-Token']).not_to be_empty + end + + it 'does not set system token header if no system token header in request' do + get url, headers: authenticated_headers + + expect(response.code).to eq '200' + expect(response.headers).not_to include('System-Token') + end + end + end end end diff --git a/spec/requests/api/connect/v3/systems/products_controller_spec.rb b/spec/requests/api/connect/v3/systems/products_controller_spec.rb index 457a77d65..9f6c47916 100644 --- a/spec/requests/api/connect/v3/systems/products_controller_spec.rb +++ b/spec/requests/api/connect/v3/systems/products_controller_spec.rb @@ -135,6 +135,26 @@ end end + shared_context 'activate with token in request headers' do + let(:payload) do + { + identifier: product.identifier, + version: product.version, + arch: product.arch, + token: regcode + } + end + + before { post url, headers: { 'System-Token' => 'existing_token' }.merge(headers), params: payload } + subject do + Struct.new(:body, :code, :headers).new( + JSON.parse(response.body, symbolize_names: true), + response.status, + response.headers + ) + end + end + context 'unknown subscription' do include_context 'with subscriptions' let(:regcode) { 'NOT-EXISTING-SUBSCRIPTION' } @@ -173,6 +193,17 @@ expect(activation.product).to eq(product) end end + + context 'token update after activation is success' do + let(:subscription) { create :subscription, :with_products } + let(:product) { subscription.products.first } + let(:regcode) { subscription.regcode } + + include_context 'activate with token in request headers' + its(:code) { is_expected.to eq(201) } + its(:headers) { is_expected.to include('System-Token') } + its(:headers['System-Token']) { is_expected.not_to eq('existing_token') } + end end end @@ -225,6 +256,18 @@ its(:body) { is_expected.to eq(serialized_json) } end + describe 'response header should contain token' do + subject { response } + + let(:token_headers) do + headers.merge({ 'System-Token' => 'some_token' }) + end + + before { get url, headers: token_headers, params: payload } + its(:code) { is_expected.to eq('200') } + its(:headers) { is_expected.to include('System-Token') } + end + describe 'response with "-" in version' do subject { response } @@ -339,6 +382,18 @@ system.reload end + it 'calls refresh_system_token after upgrade action when system token header is present' do + put url, headers: headers.merge('System-Token' => 'test_token'), params: payload + expect(response.code).to eq('201') + expect(response.headers).to include('System-Token') + expect(response.headers['System-Token']).not_to eq('test_token') + end + + it 'No update in token after upgrade action when system token header is absent' do + put url, headers: headers, params: payload + expect(response.code).to eq('201') + expect(response.headers).not_to include('System-Token') + end context 'new product' do its(:code) { is_expected.to eq('201') } its(:body) { is_expected.to eq(serialized_json) } diff --git a/spec/requests/api/connect/v3/systems/systems_controller_spec.rb b/spec/requests/api/connect/v3/systems/systems_controller_spec.rb index 95eede620..91f73d4f4 100644 --- a/spec/requests/api/connect/v3/systems/systems_controller_spec.rb +++ b/spec/requests/api/connect/v3/systems/systems_controller_spec.rb @@ -125,6 +125,25 @@ expect(system.reload.system_information_hash[:user_agent]).to be_nil end end + + context 'response header should contain token' do + let(:headers) { auth_header.merge('System-Token': 'existing-token') } + + it 'contains refreshed token in response' do + update_action + expect(response.headers).to include('System-Token') + expect(response.headers['System-Token']).not_to equal('existing-token') + end + end + + context 'response header should not contain token' do + let(:headers) { auth_header } + + it 'contains refreshed token in response' do + update_action + expect(response.headers).not_to include('System-Token') + end + end end describe '#deregister' do diff --git a/spec/requests/api/connect/v4/systems/products_controller_spec.rb b/spec/requests/api/connect/v4/systems/products_controller_spec.rb index 8bf9a64f4..6f960a941 100644 --- a/spec/requests/api/connect/v4/systems/products_controller_spec.rb +++ b/spec/requests/api/connect/v4/systems/products_controller_spec.rb @@ -140,4 +140,77 @@ end end end + + describe 'system token refresh' do + let(:system) { FactoryBot.create(:system, :with_activated_base_product) } + let(:headers) { auth_header.merge(version_header).merge('System-Token' => 'existing_token') } + + context 'token refresh for destroy action' do + let(:product) { FactoryBot.create(:product, :extension, :with_mirrored_repositories, :activated, system: system) } + let(:payload) { { identifier: product.identifier, version: product.version, arch: product.arch } } + + it 'refreshes system token when System-Token header is present' do + delete connect_systems_products_url, + headers: headers, + params: payload + + expect(response.status).to eq(200) + expect(response.headers).to include('System-Token') + expect(response.headers['System-Token']).not_to eq('existing_token') + end + + it 'does not refresh token when System-Token header is absent' do + headers_without_token = auth_header.merge(version_header) + expect_any_instance_of(described_class).not_to receive(:refresh_system_token) + + delete connect_systems_products_url, + headers: headers_without_token, + params: payload + + expect(response.status).to eq(200) + expect(response.headers).not_to include('System-Token') + end + end + + context 'token refresh for synchronize action' do + let(:path) { '/connect/systems/products/synchronize' } + + it 'refreshes system token when System-Token header is present' do + params = system.products.map do |product| + { + identifier: product.identifier, + version: product.version, + arch: product.arch, + release_type: product.release_type + } + end + post path, + params: { products: params }, + headers: headers + + expect(response.status).to eq(200) + expect(response.headers).to include('System-Token') + expect(response.headers['System-Token']).not_to eq('existing_token') + end + + it 'does not refresh token when System-Token header is absent' do + headers_without_token = auth_header.merge(version_header) + + params = system.products.map do |product| + { + identifier: product.identifier, + version: product.version, + arch: product.arch, + release_type: product.release_type + } + end + post path, + params: { products: params }, + headers: headers_without_token + + expect(response.status).to eq(200) + expect(response.headers).not_to include('System-Token') + end + end + end end From c0be53cd3e758fa7ba9b026d6d5e64c332f81c31 Mon Sep 17 00:00:00 2001 From: "parag.jain" Date: Wed, 4 Dec 2024 15:41:57 +0530 Subject: [PATCH 4/4] add system token enable check --- app/controllers/api/connect/base_controller.rb | 6 ++++-- .../api/connect/v4/systems/products_controller.rb | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/connect/base_controller.rb b/app/controllers/api/connect/base_controller.rb index 1983fb267..84de46574 100644 --- a/app/controllers/api/connect/base_controller.rb +++ b/app/controllers/api/connect/base_controller.rb @@ -49,7 +49,9 @@ def system_token_header end def refresh_system_token - @system.update(system_token: SecureRandom.uuid) - system_token_header + if system_tokens_enabled? + @system.update(system_token: SecureRandom.uuid) + system_token_header + end end end diff --git a/app/controllers/api/connect/v4/systems/products_controller.rb b/app/controllers/api/connect/v4/systems/products_controller.rb index c92df74d5..d4c87adfc 100644 --- a/app/controllers/api/connect/v4/systems/products_controller.rb +++ b/app/controllers/api/connect/v4/systems/products_controller.rb @@ -1,6 +1,6 @@ class Api::Connect::V4::Systems::ProductsController < Api::Connect::V3::Systems::ProductsController - after_action :refresh_system_token, only: %i[synchronize destroy], if: -> { request.headers.key?(SYSTEM_TOKEN_HEADER) } + after_action :refresh_system_token, only: %i[activate upgrade synchronize destroy], if: -> { request.headers.key?(SYSTEM_TOKEN_HEADER) } def destroy if @product.base? raise ActionController::TranslatedError.new(N_('The product "%s" is a base product and cannot be deactivated'), @product.name)