From 7fd1258a33af2a29488ef4bd9b17db008b2d3014 Mon Sep 17 00:00:00 2001 From: Vaibhav Goyal Date: Wed, 29 Jul 2026 17:59:46 +0530 Subject: [PATCH] OTWO-7721 Enforce API key authentication on JSON format requests --- app/controllers/application_controller.rb | 24 ++++++++++++++++---- app/controllers/kudos_controller.rb | 2 +- app/controllers/sitemap_controller.rb | 2 +- app/controllers/stacks_controller.rb | 2 +- test/controllers/accounts_controller_test.rb | 21 ++++++++++++++++- 5 files changed, 43 insertions(+), 8 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6a5de0cf2..4553c6efc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -28,7 +28,7 @@ class ApplicationController < ActionController::Base before_action :handle_me_account_paths before_action :strip_query_param before_action :clear_reminder - before_action :verify_api_access_for_xml_request, only: %i[show index similar] + before_action :verify_api_access, only: %i[show index similar] before_action :update_last_seen_at_and_ip before_action :check_maintenance_mode @@ -248,9 +248,25 @@ def check_maintenance_mode private - def verify_api_access_for_xml_request - return unless request_format == 'xml' || (params[:action] == 'similar' && request_format == 'json') - return render_missing_api_key if params[:api_key].blank? + def verify_api_access + return unless %w[xml json].include?(request_format) + + if request_format == 'json' + verify_json_api_access + else + verify_xml_api_access + end + end + + def verify_json_api_access + return if current_user.present? + return render_missing_api_key if api_client_id.blank? + + verify_api_key_standing + end + + def verify_xml_api_access + return render_missing_api_key if api_client_id.blank? verify_api_key_standing end diff --git a/app/controllers/kudos_controller.rb b/app/controllers/kudos_controller.rb index 671eca1d6..08ddcfd54 100644 --- a/app/controllers/kudos_controller.rb +++ b/app/controllers/kudos_controller.rb @@ -2,7 +2,7 @@ class KudosController < ApplicationController before_action :session_required, :redirect_unverified_account, except: %i[index sent] - before_action :verify_api_access_for_xml_request, only: %i[index sent] + before_action :verify_api_access, only: %i[index sent] before_action :find_account, only: %i[index sent] before_action :find_account_or_contribution, only: %i[new create] before_action :find_kudo, only: [:destroy] diff --git a/app/controllers/sitemap_controller.rb b/app/controllers/sitemap_controller.rb index 94d3102eb..7e29a056d 100644 --- a/app/controllers/sitemap_controller.rb +++ b/app/controllers/sitemap_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class SitemapController < ApplicationController - skip_before_action :verify_api_access_for_xml_request + skip_before_action :verify_api_access SITEMAPS = [{ ctrl: 'projects', model: Project, priority: 0.8, select: 'id, vanity_url' }, { ctrl: 'accounts', model: Account, priority: 0.6, select: 'id, login' }].freeze diff --git a/app/controllers/stacks_controller.rb b/app/controllers/stacks_controller.rb index 279f95cc1..4eb5ab911 100644 --- a/app/controllers/stacks_controller.rb +++ b/app/controllers/stacks_controller.rb @@ -12,7 +12,7 @@ class StacksController < ApplicationController before_action :auto_ignore, only: [:builder] before_action :set_project_or_fail, only: %i[near project_stacks] before_action :account_context, only: %i[index show similar] - before_action :verify_api_access_for_xml_request, only: [:project_stacks] + before_action :verify_api_access, only: [:project_stacks] def index @stacks = @account.stacks.paginate(page: page_param, per_page: 10) diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 3c65c5290..5b1c72268 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -133,6 +133,16 @@ class AccountsControllerTest < ActionController::TestCase assert_response :ok end + it 'should return bad request for json show without api key' do + get :show, params: { id: admin.login, format: :json } + assert_response :bad_request + end + + it 'should return bad request for json show with invalid api key' do + get :show, params: { id: admin.login, format: :json, api_key: 'invalid_key' } + assert_response :bad_request + end + it 'should support accounts with account_analyses' do best_account_analysis = create(:best_account_analysis) key = create(:api_key, account_id: create(:account).id) @@ -141,7 +151,16 @@ class AccountsControllerTest < ActionController::TestCase assert_response :ok end - it 'should respond to json format' do + it 'should respond to json format with api key' do + key = create(:api_key, account_id: create(:account).id) + get :show, params: { id: admin.login, format: 'json', api_key: key.oauth_application.uid } + + assert_response :ok + _(assigns(:account)).must_equal admin + end + + it 'should allow logged-in user to access json format without api key' do + login_as admin get :show, params: { id: admin.login, format: 'json' } assert_response :ok