diff --git a/app/controllers/admin/api/applications_controller.rb b/app/controllers/admin/api/applications_controller.rb index 54eef7e486..33ec6724fe 100644 --- a/app/controllers/admin/api/applications_controller.rb +++ b/app/controllers/admin/api/applications_controller.rb @@ -1,4 +1,5 @@ class Admin::Api::ApplicationsController < Admin::Api::BaseController + include ApiAuthentication::ByZyncToken representer ::Cinstance paginate only: :index diff --git a/app/controllers/admin/api/providers_controller.rb b/app/controllers/admin/api/providers_controller.rb index 3ee5b88ef7..553eead37c 100644 --- a/app/controllers/admin/api/providers_controller.rb +++ b/app/controllers/admin/api/providers_controller.rb @@ -1,4 +1,5 @@ class Admin::Api::ProvidersController < Admin::Api::BaseController + include ApiAuthentication::ByZyncToken wrap_parameters Account diff --git a/app/controllers/admin/api/services/proxies_controller.rb b/app/controllers/admin/api/services/proxies_controller.rb index 011bba9bf9..a1356334b8 100644 --- a/app/controllers/admin/api/services/proxies_controller.rb +++ b/app/controllers/admin/api/services/proxies_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Admin::Api::Services::ProxiesController < Admin::Api::Services::BaseController + include ApiAuthentication::ByZyncToken represents :json, entity: ::ProxyRepresenter::JSON represents :xml, entity: ::ProxyRepresenter::XML diff --git a/app/lib/api_authentication/by_access_token.rb b/app/lib/api_authentication/by_access_token.rb index cc114011b4..46527b439f 100644 --- a/app/lib/api_authentication/by_access_token.rb +++ b/app/lib/api_authentication/by_access_token.rb @@ -111,7 +111,7 @@ def authenticated_token token = domain_account.access_tokens.find_from_value(given_token) - return if token.blank? || token.expired? + return if token.blank? || token.expired? || token.name == AccessToken::OIDC_SYNC_TOKEN @authenticated_token = token end diff --git a/app/lib/api_authentication/by_zync_token.rb b/app/lib/api_authentication/by_zync_token.rb new file mode 100644 index 0000000000..2bee8b93e1 --- /dev/null +++ b/app/lib/api_authentication/by_zync_token.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module ApiAuthentication + module ByZyncToken + extend ActiveSupport::Concern + + included do + prepend_before_action :authenticate_zync_request, only: %i[show find] + end + + private + + def current_user + if @zync_authenticated + @current_user ||= User.current = (domain_account.find_impersonation_admin || domain_account.first_admin!) + else + super + end + end + + def authenticate_zync_request + return unless zync_request? + return if domain_account.master? + + @zync_authenticated = true + end + + def zync_request? + AuthenticatedSystem::Request.new(request).zync? + end + + # Force read-only DB transaction for Zync requests. + # ByAccessToken's version calls PermissionEnforcer.enforce(authenticated_token) — with + # no access token, authenticated_token is nil, level becomes nil, and + # requires_transaction? returns nil, skipping enforcement entirely. + def enforce_access_token_permission(&block) + if @zync_authenticated + ApiAuthentication::ByAccessToken::PermissionEnforcer.enforce(OpenStruct.new(permission: 'ro'), &block) + else + super + end + end + end +end diff --git a/app/models/access_token.rb b/app/models/access_token.rb index 7cdaeb0cb3..ee9adccee9 100644 --- a/app/models/access_token.rb +++ b/app/models/access_token.rb @@ -142,13 +142,8 @@ def self.find_from_id_or_value(id_or_value) find_by(id: id_or_value) || find_from_value(id_or_value) end - # This can't change or it will create new tokens for everyone OIDC_SYNC_TOKEN = 'OIDC Synchronization Token'.freeze - def self.oidc_sync - create_with(scopes: %w[account_management], permission: 'ro').find_or_create_by!(name: OIDC_SYNC_TOKEN) - end - def scopes=(values) super Array(values).select(&:present?) end diff --git a/app/workers/zync_worker.rb b/app/workers/zync_worker.rb index 5ff32e6acb..4ec50fe512 100644 --- a/app/workers/zync_worker.rb +++ b/app/workers/zync_worker.rb @@ -172,10 +172,8 @@ def provider_endpoint(provider) delegate :provider_access_token, to: :class - def self.provider_access_token(provider) - user = provider.find_impersonation_admin || provider.first_admin! - - user.access_tokens.oidc_sync.value + def self.provider_access_token(_provider) + 'zync' end def notification_url diff --git a/test/integration/api_authentication/by_zync_token_test.rb b/test/integration/api_authentication/by_zync_token_test.rb new file mode 100644 index 0000000000..475c3667f8 --- /dev/null +++ b/test/integration/api_authentication/by_zync_token_test.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +require 'test_helper' + +# Integration tests for ApiAuthentication::ByZyncToken. +# +# Uses a fake controller with fake routes (pattern from forbid_params_test.rb) to +# avoid coupling to real Admin API controllers and their additional before_actions. +module ApiAuthentication + module ByZyncTokenIntegration + # Minimal base that mirrors what real Admin API controllers set up: + # ApplicationController -> ByAccessToken -> ByZyncToken. + class FakeController < Admin::Api::BaseController + include ApiAuthentication::ByZyncToken + + def show + render json: { account_id: current_account.id, user_id: current_user.id } + end + + def update + render plain: 'ok' + end + end + + # Controller whose show action attempts a DB write — used to prove the + # read-only transaction blocks it. + class WriteAttemptController < FakeController + def show + User.where(id: -1).update_all(username: 'hacked') + render plain: 'ok' + rescue ActiveRecord::StatementInvalid => e + render plain: e.message, status: :forbidden + end + end + + module TestHelpers + ZYNC_TOKEN = 'test-zync-token' + + def with_test_routes + Rails.application.routes.draw do + get '/zync_test/show' => 'api_authentication/by_zync_token_integration/fake#show' + put '/zync_test/update' => 'api_authentication/by_zync_token_integration/fake#update' + get '/zync_test/write_attempt' => 'api_authentication/by_zync_token_integration/write_attempt#show' + end + yield + ensure + Rails.application.routes_reloader.reload! + end + + def zync_headers(token = ZYNC_TOKEN) + { 'X-Zync-Token' => token } + end + end + end + + class ByZyncTokenIntegrationTest < ActionDispatch::IntegrationTest + include ByZyncTokenIntegration::TestHelpers + + def setup + ThreeScale.config.stubs(:zync_authentication_token).returns(ZYNC_TOKEN) + @provider = FactoryBot.create(:provider_account) + host! @provider.external_admin_domain + end + end + + class AccessTokenTest < ByZyncTokenIntegrationTest + test 'rejects GET with no auth at all' do + with_test_routes do + get '/zync_test/show' + assert_response :forbidden + end + end + + test 'regular access token auth still works on Zync-capable endpoints' do + user = FactoryBot.create(:member, account: @provider, admin_sections: %w[partners]) + token = FactoryBot.create(:access_token, owner: user, scopes: 'account_management', permission: 'rw') + with_test_routes do + get '/zync_test/show', params: { access_token: token.plaintext_value } + assert_response :success + end + end + + test 'oidc_sync tokens are rejected even on Zync-capable endpoints' do + user = FactoryBot.create(:member, account: @provider, admin_sections: %w[partners]) + token = FactoryBot.create(:access_token, owner: user, scopes: 'account_management', + name: AccessToken::OIDC_SYNC_TOKEN) + with_test_routes do + get '/zync_test/show', params: { access_token: token.plaintext_value } + assert_response :forbidden + end + end + end + + class ZyncTokenTest < ByZyncTokenIntegrationTest + disable_transactional_fixtures! + + test 'rejects GET with an invalid X-Zync-Token' do + with_test_routes do + get '/zync_test/show', headers: zync_headers('wrong') + assert_response :forbidden + end + end + + test 'rejects requests targeting the master domain even with a valid X-Zync-Token' do + host! master_account.internal_admin_domain + with_test_routes do + get '/zync_test/show', headers: zync_headers + assert_response :forbidden + end + end + + test 'does not authenticate write actions via X-Zync-Token' do + with_test_routes do + put '/zync_test/update', headers: zync_headers + assert_response :forbidden + end + end + + test 'authenticates GET requests with a valid X-Zync-Token' do + with_test_routes do + get '/zync_test/show', headers: zync_headers + assert_response :success + end + end + + test 'Zync-authenticated requests enforce a read-only DB transaction' do + with_test_routes do + get '/zync_test/write_attempt', headers: zync_headers + assert_response :forbidden + assert_match(/read.only transaction/i, response.body) + end + end + end + + class DomainRoutingTest < ByZyncTokenIntegrationTest + disable_transactional_fixtures! + + test 'authenticates as the admin of the provider whose domain is in the Host header' do + with_test_routes do + get '/zync_test/show', headers: zync_headers + assert_response :success + assert_equal @provider.id, response.parsed_body['account_id'] + end + end + + test 'X-Forwarded-Host overrides Host header for domain resolution' do + provider_b = FactoryBot.create(:provider_account) + with_test_routes do + get '/zync_test/show', headers: zync_headers.merge('X-Forwarded-Host' => provider_b.internal_admin_domain) + assert_response :success + assert_equal provider_b.id, response.parsed_body['account_id'] + end + end + + test 'rejects master domain via X-Forwarded-Host even with valid Zync token' do + with_test_routes do + get '/zync_test/show', headers: zync_headers.merge('X-Forwarded-Host' => master_account.internal_admin_domain) + assert_response :forbidden + end + end + end +end diff --git a/test/integration/by_access_token_integration_test.rb b/test/integration/by_access_token_integration_test.rb index d7a53ade36..59c10eace5 100644 --- a/test/integration/by_access_token_integration_test.rb +++ b/test/integration/by_access_token_integration_test.rb @@ -114,4 +114,15 @@ def test_index_with_access_token assert_response :forbidden end + + test 'oidc_sync tokens are rejected even with a valid plaintext value' do + # Create a token with the oidc_sync name and store its plaintext value + token = FactoryBot.create(:access_token, owner: @user, scopes: 'account_management', + name: AccessToken::OIDC_SYNC_TOKEN) + plaintext = token.plaintext_value + + get admin_api_accounts_path(format: :xml), params: { access_token: plaintext } + + assert_response :forbidden + end end diff --git a/test/models/access_token_test.rb b/test/models/access_token_test.rb index 34f3b29c6f..735e92c514 100644 --- a/test/models/access_token_test.rb +++ b/test/models/access_token_test.rb @@ -312,6 +312,14 @@ def test_find_from_value_rejects_leaked_hash_as_token assert access_token.valid? end + test 'AccessToken.oidc_sync no longer exists' do + assert_not AccessToken.respond_to?(:oidc_sync) + end + + test 'OIDC_SYNC_TOKEN constant is still defined for rejection logic' do + assert_equal 'OIDC Synchronization Token', AccessToken::OIDC_SYNC_TOKEN + end + private def assert_access_token_audit_all_data(access_token, audit) diff --git a/test/unit/api_authentication/by_authentication_token_test.rb b/test/unit/api_authentication/by_authentication_token_test.rb index d81cf92839..c232a1fc5a 100644 --- a/test/unit/api_authentication/by_authentication_token_test.rb +++ b/test/unit/api_authentication/by_authentication_token_test.rb @@ -22,7 +22,7 @@ def setup def mock_token(attributes = {}) @params = { access_token: 'some-token' } - token = mock('access-token', attributes.merge(expired?: false)) + token = mock('access-token', **attributes, expired?: false, name: 'access-token') @access_tokens.expects(:find_from_value).with('some-token').returns(token) token end diff --git a/test/unit/api_authentication/by_zync_token_test.rb b/test/unit/api_authentication/by_zync_token_test.rb new file mode 100644 index 0000000000..5e1b565b52 --- /dev/null +++ b/test/unit/api_authentication/by_zync_token_test.rb @@ -0,0 +1,109 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ApiAuthentication::ByZyncTokenTest < ActiveSupport::TestCase + # Minimal host object that includes ByZyncToken so we can call its methods directly. + class Host + include ActiveSupport::Callbacks + define_callbacks :action + include ActiveSupport::Rescuable + include AbstractController::Callbacks + extend AbstractController::Callbacks::ClassMethods + + include ApiAuthentication::ByAccessToken + include ApiAuthentication::ByZyncToken + + attr_reader :request, :domain_account, :params + + def initialize(request:, domain_account:) + @request = request + @domain_account = domain_account + @params = {} + end + end + + def setup + @user = stub('user') + @account = stub('account', master?: false, + find_impersonation_admin: nil, + first_admin!: @user) + @request = stub('request', authorization: nil) + @host = Host.new(request: @request, domain_account: @account) + end + + # authenticate_zync_request + + test '#authenticate_zync_request sets zync_authenticated flag for a valid Zync request' do + AuthenticatedSystem::Request.stubs(:new).with(@request).returns(stub(zync?: true)) + + @host.send(:authenticate_zync_request) + + assert @host.instance_variable_get(:@zync_authenticated) + end + + test '#authenticate_zync_request is a no-op when X-Zync-Token is wrong' do + AuthenticatedSystem::Request.stubs(:new).with(@request).returns(stub(zync?: false)) + + @host.send(:authenticate_zync_request) + + assert_nil @host.instance_variable_get(:@zync_authenticated) + end + + test '#authenticate_zync_request is a no-op when domain is master' do + AuthenticatedSystem::Request.stubs(:new).with(@request).returns(stub(zync?: true)) + @account.stubs(:master?).returns(true) + + @host.send(:authenticate_zync_request) + + assert_nil @host.instance_variable_get(:@zync_authenticated) + end + + # current_user + + test '#current_user falls back to first_admin! when no impersonation admin exists' do + AuthenticatedSystem::Request.stubs(:new).with(@request).returns(stub(zync?: true)) + @host.send(:authenticate_zync_request) + + assert_equal @user, @host.send(:current_user) + end + + test '#current_user prefers the impersonation admin over first_admin!' do + impersonation_admin = stub('impersonation_admin') + @account.stubs(:find_impersonation_admin).returns(impersonation_admin) + AuthenticatedSystem::Request.stubs(:new).with(@request).returns(stub(zync?: true)) + @host.send(:authenticate_zync_request) + + assert_equal impersonation_admin, @host.send(:current_user) + end + + test '#current_user delegates to super for non-Zync requests' do + AuthenticatedSystem::Request.stubs(:new).with(@request).returns(stub(zync?: false)) + @host.send(:authenticate_zync_request) + + assert_nil @host.send(:current_user) + end + + # enforce_access_token_permission + + class EnforcePermissionTest < ApiAuthentication::ByZyncTokenTest + # Read-only transaction enforcement can't run inside a transaction. + self.use_transactional_tests = false + + test '#enforce_access_token_permission enforces a read-only DB transaction for Zync requests' do + @host.instance_variable_set(:@zync_authenticated, true) + + assert_raises ApiAuthentication::ByAccessToken::PermissionError do + @host.send(:enforce_access_token_permission) { User.delete_all } + end + end + end + + test '#enforce_access_token_permission delegates to ByAccessToken for non-Zync requests' do + # @zync_authenticated is not set — falls through to ByAccessToken's version. + # With no authenticated_token (no access_token param, no HTTP auth) it yields normally. + executed = false + @host.send(:enforce_access_token_permission) { executed = true } + assert executed + end +end diff --git a/test/workers/zync_worker_test.rb b/test/workers/zync_worker_test.rb index 60d5acb53b..f1e5c44aa4 100644 --- a/test/workers/zync_worker_test.rb +++ b/test/workers/zync_worker_test.rb @@ -222,4 +222,11 @@ class UnprocessableEntityRetryTest < ActiveSupport::TestCase assert_equal zync_event.data[:service_id], dependency_event_service.data[:id] Sidekiq::Testing.inline! { ZyncWorker.perform_async( dependency_event_service.event_id, dependency_event_service.data.as_json) } end + + test 'provider_access_token returns a non-empty placeholder string' do + provider = FactoryBot.create(:provider_account) + token = ZyncWorker.provider_access_token(provider) + assert_kind_of String, token + assert token.present? + end end