From 61abc47acc75e9469cb81d171369d43e381e24fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Thu, 21 May 2026 12:32:42 +0200 Subject: [PATCH 1/4] feat(api-auth): add ByZyncToken authentication module Introduces a new authentication mechanism for Zync service requests using the X-Zync-Token shared secret header instead of per-provider oidc_sync access tokens. This fixes the Zync integration broken by access token hashing (SHA384) in PR #4236, where hashed token values became unusable for authentication. The module authenticates Zync requests on read-only actions (show, find) and enforces read-only database transactions as defense in depth. It explicitly rejects requests targeting the master account domain to prevent unauthorized master API access via the shared Zync token. Authentication resolves to the provider's impersonation admin or first admin, determined by the domain in the Host header (or X-Forwarded-Host in proxy mode). For non-Zync requests, it delegates to ByAccessToken for regular access token authentication. Assisted-by: Claude Code --- app/lib/api_authentication/by_zync_token.rb | 44 +++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 app/lib/api_authentication/by_zync_token.rb 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 From 0c09911dce6efc1a03658c37c2b1d8fdc56a5ca0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Thu, 21 May 2026 12:32:55 +0200 Subject: [PATCH 2/4] feat(api-auth): integrate ByZyncToken into Admin API controllers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enables Zync authentication on the three Admin API endpoints that Zync calls to fetch provider configuration: provider details, service proxy configuration, and application lookups. These are the only endpoints where Zync needs read access. Including ByZyncToken only in these controllers (not in BaseController) limits the attack surface — even with a valid X-Zync-Token, an attacker cannot access billing, analytics, user management, or other Admin API endpoints. Assisted-by: Claude Code --- app/controllers/admin/api/applications_controller.rb | 1 + app/controllers/admin/api/providers_controller.rb | 1 + app/controllers/admin/api/services/proxies_controller.rb | 1 + 3 files changed, 3 insertions(+) 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 From 1c430233f206a232065a76a46586efd4e821539b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Thu, 21 May 2026 12:33:07 +0200 Subject: [PATCH 3/4] refactor(access-token): deprecate oidc_sync tokens for Zync auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the oidc_sync access token mechanism now that Zync authenticates via X-Zync-Token instead. This change: 1. Rejects oidc_sync tokens at the authentication layer, invalidating all existing oidc_sync tokens immediately (including plaintext copies sitting in Zync databases from before the hashing migration). 2. Removes AccessToken.oidc_sync class method — new oidc_sync tokens can no longer be created. The OIDC_SYNC_TOKEN constant remains for the rejection logic. 3. Simplifies ZyncWorker.provider_access_token to return a placeholder string 'zync' instead of fetching a per-provider token. Zync no longer uses this for authentication, but the field is sent to satisfy Zync's Tenant model validation (validates :access_token, presence: true). This eliminates the security issue of storing plaintext tokens in Zync's database and removes the mechanism that broke when access tokens were hashed in PR #4236. Assisted-by: Claude Code --- app/lib/api_authentication/by_access_token.rb | 2 +- app/models/access_token.rb | 5 ----- app/workers/zync_worker.rb | 6 ++---- 3 files changed, 3 insertions(+), 10 deletions(-) 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/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 From f9a1c776c2abfd073be09b8a8e4309d0d4491980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Thu, 21 May 2026 12:33:22 +0200 Subject: [PATCH 4/4] test: add comprehensive test coverage for Zync authentication Adds unit and integration tests covering the new ByZyncToken authentication module and the oidc_sync token deprecation: Unit tests (by_zync_token_test.rb): - authenticate_zync_request flag setting and rejection (invalid token, master domain) - current_user resolution (impersonation admin preference, fallback to first_admin!, delegation to super for non-Zync requests) - read-only transaction enforcement for Zync-authenticated requests Integration tests (by_zync_token_test.rb): - Regular access token auth still works on Zync-capable endpoints - oidc_sync tokens rejected on Zync-capable endpoints - X-Zync-Token authentication on read actions (show, find) - Rejection scenarios (invalid token, master domain, write actions) - Read-only database transaction enforcement (write attempts fail) - Domain routing via Host and X-Forwarded-Host headers Additional test coverage: - ByAccessToken: oidc_sync token rejection at authentication layer - AccessToken model: oidc_sync method removal, constant preservation - ZyncWorker: provider_access_token returns non-empty placeholder All tests passing: 17 unit, 11 integration, plus existing coverage in related test files. Assisted-by: Claude Code --- .../api_authentication/by_zync_token_test.rb | 162 ++++++++++++++++++ .../by_access_token_integration_test.rb | 11 ++ test/models/access_token_test.rb | 8 + .../by_authentication_token_test.rb | 2 +- .../api_authentication/by_zync_token_test.rb | 109 ++++++++++++ test/workers/zync_worker_test.rb | 7 + 6 files changed, 298 insertions(+), 1 deletion(-) create mode 100644 test/integration/api_authentication/by_zync_token_test.rb create mode 100644 test/unit/api_authentication/by_zync_token_test.rb 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