-
Notifications
You must be signed in to change notification settings - Fork 71
THREESCALE-14969: Allow authentication by zync token #4304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
61abc47
0c09911
1c43023
f9a1c77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The zync authenticated actions, in particular, setting |
||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
|
Comment on lines
-148
to
-151
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We stop creating new OIDC tokens |
||
| def scopes=(values) | ||
| super Array(values).select(&:present?) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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' | ||
|
Comment on lines
+175
to
+176
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super fan of this, but the |
||
| end | ||
|
|
||
| def notification_url | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line invalidates all OIDC access tokens from now on, so it doesn't matter if they still sit plain text in the Zync DB.