From 6c8e518df7b9c77545bdc4c08b24c47d8159ae66 Mon Sep 17 00:00:00 2001 From: Josh Pigford Date: Tue, 17 Jun 2025 05:40:58 -0500 Subject: [PATCH] Enhance API authentication and onboarding handling - Updated BaseController to skip onboarding requirements for API endpoints and added manual token verification for OAuth authentication. - Improved error handling and logging for invalid access tokens. - Introduced a method to set up the current context for API requests, ensuring compatibility with session-like behavior. - Excluded API paths from onboarding redirects in the Onboardable concern. - Updated database schema to change resource_owner_id type from bigint to string for OAuth access grants. --- app/controllers/api/v1/base_controller.rb | 83 +++++++++++++++++-- app/controllers/concerns/onboardable.rb | 1 + ...ccess_grants_resource_owner_id_for_uuid.rb | 9 ++ db/schema.rb | 4 +- 4 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20250613152743_fix_doorkeeper_access_grants_resource_owner_id_for_uuid.rb diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 0a447375..76883150 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -6,13 +6,18 @@ class Api::V1::BaseController < ApplicationController # Skip regular session-based authentication for API skip_authentication - # Force JSON format for all API requests + # Skip onboarding requirements for API endpoints + skip_before_action :require_onboarding_and_upgrade + + # Force JSON format for all API requests before_action :force_json_format # Use our custom authentication that supports both OAuth and API keys before_action :authenticate_request! before_action :check_api_key_rate_limit before_action :log_api_access + + # Override Doorkeeper's default behavior to return JSON instead of redirecting def doorkeeper_unauthorized_render_options(error: nil) { json: { error: "unauthorized", message: "Access token is invalid, expired, or missing" } } @@ -30,20 +35,52 @@ class Api::V1::BaseController < ApplicationController request.format = :json end - # Authenticate using either OAuth or API key + # Authenticate using either OAuth or API key def authenticate_request! - authenticate_oauth || authenticate_api_key || render_unauthorized + return if authenticate_oauth + return if authenticate_api_key + render_unauthorized unless performed? end # Try OAuth authentication first def authenticate_oauth return false unless request.headers["Authorization"].present? - doorkeeper_authorize! - @current_user = User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token - @authentication_method = :oauth - true - rescue Doorkeeper::Errors::DoorkeeperError + # Manually verify the token (bypassing doorkeeper_authorize! which had scope issues) + token_string = request.authorization&.split(' ')&.last + access_token = Doorkeeper::AccessToken.by_token(token_string) + + # Check token validity and scope (read_write includes read access) + has_sufficient_scope = access_token&.scopes&.include?('read') || access_token&.scopes&.include?('read_write') + + unless access_token && !access_token.expired? && has_sufficient_scope + render_json({ error: "unauthorized", message: "Access token is invalid, expired, or missing required scope" }, status: :unauthorized) + return false + end + + # Set the doorkeeper_token for compatibility + @_doorkeeper_token = access_token + + if doorkeeper_token&.resource_owner_id + @current_user = User.find_by(id: doorkeeper_token.resource_owner_id) + + # If user doesn't exist, the token is invalid (user was deleted) + unless @current_user + Rails.logger.warn "API OAuth Token Invalid: Access token resource_owner_id #{doorkeeper_token.resource_owner_id} does not exist" + render_json({ error: "unauthorized", message: "Access token is invalid - user not found" }, status: :unauthorized) + return false + end + else + Rails.logger.warn "API OAuth Token Invalid: Access token missing resource_owner_id" + render_json({ error: "unauthorized", message: "Access token is invalid - missing resource owner" }, status: :unauthorized) + return false + end + + @authentication_method = :oauth + setup_current_context_for_api + true + rescue Doorkeeper::Errors::DoorkeeperError => e + Rails.logger.warn "API OAuth Error: #{e.message}" false end @@ -59,6 +96,7 @@ class Api::V1::BaseController < ApplicationController @api_key.update_last_used! @authentication_method = :api_key @rate_limiter = ApiRateLimiter.new(@api_key) + setup_current_context_for_api true end @@ -129,6 +167,7 @@ class Api::V1::BaseController < ApplicationController end # Check if the current authentication has the required scope + # Implements hierarchical scope checking where read_write includes read access def authorize_scope!(required_scope) scopes = current_scopes @@ -189,7 +228,7 @@ class Api::V1::BaseController < ApplicationController Rails.logger.info "API Request: #{request.method} #{request.path} - User: #{current_resource_owner.email} (Family: #{current_resource_owner.family_id}) - Auth: #{auth_info}" end - # Family-based access control helper (to be used by subcontrollers) + # Family-based access control helper (to be used by subcontrollers) def ensure_current_family_access(resource) return unless resource.respond_to?(:family_id) @@ -201,4 +240,30 @@ class Api::V1::BaseController < ApplicationController true end + + # Manual doorkeeper_token accessor for compatibility with manual token verification + def doorkeeper_token + @_doorkeeper_token + end + + # Set up Current context for API requests since we don't use session-based auth + def setup_current_context_for_api + # For API requests, we need to create a minimal session-like object + # or find/create an actual session for this user to make Current.user work + if @current_user + # Try to find an existing session for this user, or create a temporary one + session = @current_user.sessions.first + if session + Current.session = session + else + # Create a temporary session for this API request + # This won't be persisted but will allow Current.user to work + session = @current_user.sessions.build( + user_agent: request.user_agent, + ip_address: request.ip + ) + Current.session = session + end + end + end end diff --git a/app/controllers/concerns/onboardable.rb b/app/controllers/concerns/onboardable.rb index 9d1f9ef6..60655094 100644 --- a/app/controllers/concerns/onboardable.rb +++ b/app/controllers/concerns/onboardable.rb @@ -25,6 +25,7 @@ module Onboardable return false if path.starts_with?("/subscription") return false if path.starts_with?("/onboarding") return false if path.starts_with?("/users") + return false if path.starts_with?("/api") # Exclude API endpoints from onboarding redirects [ new_registration_path, diff --git a/db/migrate/20250613152743_fix_doorkeeper_access_grants_resource_owner_id_for_uuid.rb b/db/migrate/20250613152743_fix_doorkeeper_access_grants_resource_owner_id_for_uuid.rb new file mode 100644 index 00000000..f23b0ec8 --- /dev/null +++ b/db/migrate/20250613152743_fix_doorkeeper_access_grants_resource_owner_id_for_uuid.rb @@ -0,0 +1,9 @@ +class FixDoorkeeperAccessGrantsResourceOwnerIdForUuid < ActiveRecord::Migration[7.2] + def up + change_column :oauth_access_grants, :resource_owner_id, :string + end + + def down + change_column :oauth_access_grants, :resource_owner_id, :bigint + end +end diff --git a/db/schema.rb b/db/schema.rb index 41b98dc7..95ed2ec6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_06_13_101051) do +ActiveRecord::Schema[7.2].define(version: 2025_06_13_152743) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -430,7 +430,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_06_13_101051) do end create_table "oauth_access_grants", force: :cascade do |t| - t.bigint "resource_owner_id", null: false + t.string "resource_owner_id", null: false t.bigint "application_id", null: false t.string "token", null: false t.integer "expires_in", null: false