From b3f70c8951e8f5a50f8f3fef1e3f5fbb29ee92e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Jos=C3=A9=20Mata?= Date: Mon, 29 Jun 2026 00:30:03 +0200 Subject: [PATCH] feat:Add SnapTrade OAuth device flow (#2523) * Add SnapTrade OAuth connection flow * Restore SnapTrade brokerage portal links * Guard SnapTrade OAuth setup completion * Move SnapTrade OAuth start to POST * Use one SnapTrade item in provider panel * Fix SnapTrade OAuth controller tests * Fix SnapTrade OAuth drawer completion redirect * Update SnapTrade limits message. * Restrict SnapTrade OAuth scopes --- app/controllers/snaptrade_items_controller.rb | 188 +++++++++++++-- app/helpers/settings_helper.rb | 3 +- app/models/provider/snaptrade.rb | 19 +- app/models/snaptrade_item.rb | 8 +- app/models/snaptrade_item/provided.rb | 14 +- .../providers/_snaptrade_panel.html.erb | 139 ++++++++--- .../snaptrade_items/_snaptrade_item.html.erb | 14 +- .../oauth_device_flow.html.erb | 112 +++++++++ .../snaptrade_items/setup_accounts.html.erb | 18 +- config/locales/views/snaptrade_items/en.yml | 27 ++- config/routes.rb | 2 + .../snaptrade_items_controller_test.rb | 224 +++++++++++++++++- test/models/snaptrade_item_test.rb | 9 +- 13 files changed, 681 insertions(+), 96 deletions(-) create mode 100644 app/views/snaptrade_items/oauth_device_flow.html.erb diff --git a/app/controllers/snaptrade_items_controller.rb b/app/controllers/snaptrade_items_controller.rb index 2406f2768..ab3733e20 100644 --- a/app/controllers/snaptrade_items_controller.rb +++ b/app/controllers/snaptrade_items_controller.rb @@ -1,6 +1,8 @@ class SnaptradeItemsController < ApplicationController + PERMITTED_OAUTH_SCOPES = %w[read].freeze + before_action :set_snaptrade_item, only: [ :show, :edit, :update, :destroy, :sync, :connect, :setup_accounts, :complete_account_setup, :connections, :start_oauth_device_flow, :complete_oauth_device_flow, :delete_connection, :delete_orphaned_user ] - before_action :require_admin!, only: [ :new, :create, :preload_accounts, :select_accounts, :link_accounts, :select_existing_account, :link_existing_account, :edit, :update, :destroy, :sync, :connect, :callback, :setup_accounts, :complete_account_setup, :connections, :start_oauth_device_flow, :complete_oauth_device_flow, :delete_connection, :delete_orphaned_user ] + before_action :require_admin!, only: [ :new, :create, :preload_accounts, :select_accounts, :link_accounts, :select_existing_account, :link_existing_account, :oauth_connect, :start_oauth_connect, :edit, :update, :destroy, :sync, :connect, :callback, :setup_accounts, :complete_account_setup, :connections, :start_oauth_device_flow, :complete_oauth_device_flow, :delete_connection, :delete_orphaned_user ] def index @snaptrade_items = Current.family.snaptrade_items.ordered @@ -22,11 +24,13 @@ class SnaptradeItemsController < ApplicationController if @snaptrade_item.save # Register user with SnapTrade after saving credentials - begin - @snaptrade_item.ensure_user_registered! - rescue => e - Rails.logger.error "SnapTrade user registration failed: #{e.message}" - # Don't fail the whole operation - user can retry connection later + if @snaptrade_item.credentials_configured? + begin + @snaptrade_item.ensure_user_registered! + rescue => e + Rails.logger.error "SnapTrade user registration failed: #{e.message}" + # Don't fail the whole operation - user can retry connection later + end end if turbo_frame_request? @@ -162,7 +166,7 @@ class SnaptradeItemsController < ApplicationController latest_sync = @snaptrade_item.syncs.ordered.first should_sync = latest_sync.nil? || !latest_sync.completed? - if no_accounts && !@snaptrade_item.syncing? && should_sync + if @snaptrade_item.user_registered? && no_accounts && !@snaptrade_item.syncing? && should_sync @snaptrade_item.sync_later end @@ -257,8 +261,62 @@ class SnaptradeItemsController < ApplicationController } end + def oauth_connect + assign_oauth_connect_context + + unless Provider::Snaptrade.oauth_client_id_configured? + @error_message = snaptrade_oauth_client_id_missing_message + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + return + end + + @snaptrade_item = if params[:item_id].present? + Current.family.snaptrade_items.find(params[:item_id]) + else + current_snaptrade_item + end + + render :oauth_device_flow + rescue ActiveRecord::Encryption::Errors::Decryption => e + Rails.logger.error "SnapTrade decryption error for item #{@snaptrade_item&.id}: #{e.class} - #{e.message}" + @error_message = t("snaptrade_items.connect.decryption_failed") + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + rescue ActiveRecord::ActiveRecordError, ActiveRecord::Encryption::Errors::Base => e + Rails.logger.error "SnapTrade OAuth connect error: #{e.class} - #{e.message}" + @error_message = start_oauth_device_flow_error_message + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + end + + def start_oauth_connect + assign_oauth_connect_context + + unless Provider::Snaptrade.oauth_client_id_configured? + @error_message = snaptrade_oauth_client_id_missing_message + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + return + end + + @snaptrade_item = if params[:item_id].present? + Current.family.snaptrade_items.find(params[:item_id]) + else + current_snaptrade_item || Current.family.snaptrade_items.create!(name: t("snaptrade_items.default_name")) + end + + @device_authorization = @snaptrade_item.start_oauth_device_flow(scope: @oauth_scope) + + render :oauth_device_flow + rescue ActiveRecord::Encryption::Errors::Decryption => e + Rails.logger.error "SnapTrade decryption error for item #{@snaptrade_item&.id}: #{e.class} - #{e.message}" + @error_message = t("snaptrade_items.connect.decryption_failed") + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + rescue Provider::Snaptrade::Error, ActiveRecord::ActiveRecordError, ActiveRecord::Encryption::Errors::Base => e + Rails.logger.error "SnapTrade OAuth connect error: #{e.class} - #{e.message}" + @error_message = oauth_connect_error_message(e) + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + end + def start_oauth_device_flow - render json: @snaptrade_item.start_oauth_device_flow(scope: params[:scope].presence || "read") + render json: @snaptrade_item.start_oauth_device_flow(scope: permitted_oauth_scope) rescue ActiveRecord::Encryption::Errors::Decryption => e Rails.logger.error "SnapTrade decryption error for item #{@snaptrade_item.id}: #{e.class} - #{e.message}" render json: { error: t("snaptrade_items.connect.decryption_failed") }, status: :unprocessable_entity @@ -274,17 +332,42 @@ class SnaptradeItemsController < ApplicationController end token_response = @snaptrade_item.complete_oauth_device_flow!(device_code: params[:device_code]) - render json: { - token_type: token_response["token_type"], - scope: token_response["scope"], - expires_in: token_response["expires_in"], - expires_at: @snaptrade_item.oauth_token_expires_at&.iso8601 - } + if request.format.json? + render json: { + token_type: token_response["token_type"], + scope: token_response["scope"], + expires_in: token_response["expires_in"], + expires_at: @snaptrade_item.oauth_token_expires_at&.iso8601 + } + else + if prepare_snaptrade_item_for_setup_after_oauth + redirect_after_oauth_completion setup_accounts_snaptrade_item_path( + @snaptrade_item, + accountable_type: params[:accountable_type].presence, + return_to: params[:return_to].presence + ), notice: t(".success", default: "SnapTrade authorization complete.") + else + redirect_after_oauth_completion settings_providers_path, alert: snaptrade_oauth_setup_incomplete_message + end + end rescue Provider::Snaptrade::ApiError => e - render json: oauth_error_payload(e), status: e.status_code || :unprocessable_entity + if request.format.json? + render json: oauth_error_payload(e), status: e.status_code || :unprocessable_entity + else + payload = oauth_error_payload(e) + @error_message = payload["error_description"].presence || payload["error"] + restore_device_authorization_from_params + render :oauth_device_flow, status: e.status_code || :unprocessable_entity, formats: :html + end rescue Provider::Snaptrade::Error, ActiveRecord::ActiveRecordError, ActiveRecord::Encryption::Errors::Base => e Rails.logger.error "SnapTrade OAuth device token error: #{e.class} - #{e.message}" - render json: { error: complete_oauth_device_flow_error_message }, status: :unprocessable_entity + if request.format.json? + render json: { error: complete_oauth_device_flow_error_message }, status: :unprocessable_entity + else + @error_message = complete_oauth_device_flow_error_message + restore_device_authorization_from_params + render :oauth_device_flow, status: :unprocessable_entity, formats: :html + end end # Delete a brokerage connection @@ -387,7 +470,7 @@ class SnaptradeItemsController < ApplicationController snaptrade_item.sync_later unless snaptrade_item.syncing? redirect_to setup_accounts_snaptrade_item_path(snaptrade_item) else - redirect_to connect_snaptrade_item_path(snaptrade_item) + redirect_to oauth_connect_snaptrade_items_path(item_id: snaptrade_item.id) end end @@ -405,7 +488,7 @@ class SnaptradeItemsController < ApplicationController redirect_to setup_accounts_snaptrade_item_path(snaptrade_item, accountable_type: @accountable_type, return_to: @return_to) else store_snaptrade_resume_context(return_to: @return_to, accountable_type: @accountable_type) - redirect_to connect_snaptrade_item_path(snaptrade_item) + redirect_to oauth_connect_snaptrade_items_path(item_id: snaptrade_item.id, accountable_type: @accountable_type, return_to: @return_to) end end @@ -485,6 +568,53 @@ class SnaptradeItemsController < ApplicationController [ resume[:return_to], resume[:accountable_type] ] end + def restore_device_authorization_from_params + @device_authorization = params.permit( + :device_code, + :user_code, + :verification_uri, + :verification_uri_complete, + :expires_in, + :interval + ).to_h + @return_to = params[:return_to] + @accountable_type = params[:accountable_type] + end + + def assign_oauth_connect_context + @return_to = params[:return_to] + @accountable_type = params[:accountable_type] + @oauth_scope = permitted_oauth_scope + end + + def permitted_oauth_scope + requested_scope = params[:scope].to_s + return requested_scope if PERMITTED_OAUTH_SCOPES.include?(requested_scope) + + "read" + end + + def prepare_snaptrade_item_for_setup_after_oauth + if !@snaptrade_item.user_registered? && @snaptrade_item.credentials_configured? + @snaptrade_item.ensure_user_registered! + end + + return false unless @snaptrade_item.user_registered? + + @snaptrade_item.sync_later unless @snaptrade_item.syncing? + true + end + + def redirect_after_oauth_completion(path, notice: nil, alert: nil) + if turbo_frame_request? + flash[:notice] = notice if notice.present? + flash[:alert] = alert if alert.present? + render turbo_stream: turbo_stream.action(:redirect, path) + else + redirect_to path, notice: notice, alert: alert + end + end + def snaptrade_item_params params.require(:snaptrade_item).permit( :name, @@ -551,6 +681,28 @@ class SnaptradeItemsController < ApplicationController ) end + def oauth_connect_error_message(error) + if error.is_a?(Provider::Snaptrade::ConfigurationError) && error.message.include?("OAuth client ID") + snaptrade_oauth_client_id_missing_message + else + start_oauth_device_flow_error_message + end + end + + def snaptrade_oauth_client_id_missing_message + t( + "snaptrade_items.oauth_device_flow.missing_client_id", + default: "SnapTrade OAuth client ID is not configured. Add SNAPTRADE_OAUTH_CLIENT_ID to .env.local, restart the app, then try again." + ) + end + + def snaptrade_oauth_setup_incomplete_message + t( + "snaptrade_items.complete_oauth_device_flow.setup_incomplete", + default: "SnapTrade authorization is complete, but API credentials are required before accounts can sync." + ) + end + def complete_oauth_device_flow_error_message t( "snaptrade_items.complete_oauth_device_flow.failed", diff --git a/app/helpers/settings_helper.rb b/app/helpers/settings_helper.rb index 658b9199d..b88499aae 100644 --- a/app/helpers/settings_helper.rb +++ b/app/helpers/settings_helper.rb @@ -91,8 +91,9 @@ module SettingsHelper return { status: :off } unless @kraken_items&.any? sync_based_summary(key) when "snaptrade" - configured_item = @snaptrade_items&.find(&:credentials_configured?) + configured_item = @snaptrade_items&.find { |item| item.credentials_configured? || item.oauth_configured? } return { status: :off } unless configured_item + unless configured_item.user_registered? return { status: :warn, meta: t("settings.providers.meta.registration_needed") } end diff --git a/app/models/provider/snaptrade.rb b/app/models/provider/snaptrade.rb index c01be5cf1..fabe507e6 100644 --- a/app/models/provider/snaptrade.rb +++ b/app/models/provider/snaptrade.rb @@ -19,21 +19,26 @@ class Provider::Snaptrade OAUTH_DISCOVERY_URL = "https://api.snaptrade.com/.well-known/oauth-authorization-server".freeze DEVICE_CODE_GRANT = "urn:ietf:params:oauth:grant-type:device_code".freeze - attr_reader :client, :client_id, :consumer_key - - def initialize(client_id:, consumer_key:) - raise ConfigurationError, "client_id is required" if client_id.blank? - raise ConfigurationError, "consumer_key is required" if consumer_key.blank? + attr_reader :client_id, :consumer_key + def initialize(client_id: nil, consumer_key: nil) @client_id = client_id @consumer_key = consumer_key + return if client_id.blank? && consumer_key.blank? + raise ConfigurationError, "client_id is required" if client_id.blank? + raise ConfigurationError, "consumer_key is required" if consumer_key.blank? + configuration = SnapTrade::Configuration.new configuration.client_id = client_id configuration.consumer_key = consumer_key @client = SnapTrade::Client.new(configuration) end + def self.oauth_client_id_configured? + Rails.configuration.x.snaptrade&.oauth_client_id.present? + end + def oauth_authorization_server_metadata with_retries("oauth_authorization_server_metadata") do response = oauth_connection.get(OAUTH_DISCOVERY_URL) @@ -278,6 +283,10 @@ class Provider::Snaptrade private + def client + @client || raise(ConfigurationError, "SnapTrade API credentials are required") + end + def handle_api_error(error, operation) status = error.code body = error.response_body diff --git a/app/models/snaptrade_item.rb b/app/models/snaptrade_item.rb index e3e7e0249..171cdbaae 100644 --- a/app/models/snaptrade_item.rb +++ b/app/models/snaptrade_item.rb @@ -25,8 +25,8 @@ class SnaptradeItem < ApplicationRecord end validates :name, presence: true - validates :client_id, presence: true, on: :create - validates :consumer_key, presence: true, on: :create + validates :client_id, presence: true, if: -> { consumer_key.present? } + validates :consumer_key, presence: true, if: -> { client_id.present? } # Note: snaptrade_user_id and snaptrade_user_secret are populated after user registration # via ensure_user_registered!, so we don't validate them on create @@ -181,6 +181,10 @@ class SnaptradeItem < ApplicationRecord client_id.present? && consumer_key.present? end + def oauth_configured? + oauth_access_token.present? + end + # Override Syncable#syncing? to also show syncing state when activities are being # fetched in the background. This ensures the UI shows the spinner until all data # is truly imported, not just when the main sync job completes. diff --git a/app/models/snaptrade_item/provided.rb b/app/models/snaptrade_item/provided.rb index 7382a2ccf..b2bc91dec 100644 --- a/app/models/snaptrade_item/provided.rb +++ b/app/models/snaptrade_item/provided.rb @@ -14,6 +14,10 @@ module SnaptradeItem::Provided ) end + def oauth_snaptrade_provider + snaptrade_provider || Provider::Snaptrade.new + end + # Clean up SnapTrade user when item is destroyed def delete_snaptrade_user return unless user_registered? @@ -133,17 +137,11 @@ module SnaptradeItem::Provided end def start_oauth_device_flow(scope: "read") - provider = snaptrade_provider - raise Provider::Snaptrade::ConfigurationError, "SnapTrade provider not configured" unless provider - - provider.start_device_authorization(scope: scope) + oauth_snaptrade_provider.start_device_authorization(scope: scope) end def complete_oauth_device_flow!(device_code:) - provider = snaptrade_provider - raise Provider::Snaptrade::ConfigurationError, "SnapTrade provider not configured" unless provider - - token_response = provider.poll_device_token(device_code: device_code) + token_response = oauth_snaptrade_provider.poll_device_token(device_code: device_code) update!( oauth_access_token: token_response["access_token"], oauth_refresh_token: token_response["refresh_token"], diff --git a/app/views/settings/providers/_snaptrade_panel.html.erb b/app/views/settings/providers/_snaptrade_panel.html.erb index 1c814b234..565e89724 100644 --- a/app/views/settings/providers/_snaptrade_panel.html.erb +++ b/app/views/settings/providers/_snaptrade_panel.html.erb @@ -1,52 +1,116 @@
<%= render DS::Alert.new(message: t("providers.snaptrade.free_tier_warning"), variant: :warning) %> - <%= render "settings/providers/setup_steps", - steps: [ - t("providers.snaptrade.step_1_html").html_safe, - t("providers.snaptrade.step_2"), - t("providers.snaptrade.step_3"), - t("providers.snaptrade.step_4") - ] %> - <% error_msg = local_assigns[:error_message] || @error_message %> <% if error_msg.present? %> <%= render DS::Alert.new(message: error_msg, variant: :error) %> <% end %> <% - snaptrade_item = Current.family.snaptrade_items.first_or_initialize(name: "SnapTrade Connection") + items = local_assigns[:snaptrade_items] || @snaptrade_items || Current.family.snaptrade_items.active.ordered + active_items = items.select { |item| !item.scheduled_for_deletion? } + snaptrade_item = + active_items.first || + Current.family.snaptrade_items.build(name: "SnapTrade Connection") is_new_record = snaptrade_item.new_record? - is_configured = snaptrade_item.persisted? && snaptrade_item.credentials_configured? - is_registered = snaptrade_item.persisted? && snaptrade_item.user_registered? + oauth_href = + if snaptrade_item.persisted? + oauth_connect_snaptrade_items_path(item_id: snaptrade_item.id) + else + oauth_connect_snaptrade_items_path + end + oauth_active = snaptrade_item.persisted? && snaptrade_item.oauth_token_active? + oauth_button_text = + if oauth_active + t("providers.snaptrade.oauth_reauthorize_button", default: "Reauthorize") + else + t("providers.snaptrade.oauth_connect_button", default: "Authorize") + end %> - <%= styled_form_with model: snaptrade_item, - url: is_new_record ? snaptrade_items_path : snaptrade_item_path(snaptrade_item), - scope: :snaptrade_item, - method: is_new_record ? :post : :patch, - data: { turbo: true }, - class: "space-y-3" do |form| %> - <%= form.text_field :client_id, - label: t("providers.snaptrade.client_id_label"), - placeholder: is_new_record ? t("providers.snaptrade.client_id_placeholder") : t("providers.snaptrade.client_id_update_placeholder"), - type: :password %> +
+
+ + <%= icon "key-round", class: "w-4 h-4 text-success" %> + +
+

<%= t("providers.snaptrade.oauth_title", default: "SnapTrade OAuth") %>

+

+ <% if oauth_active %> + <%= t("providers.snaptrade.oauth_status_authorized", default: "Authorized for SnapTrade.") %> + <% else %> + <%= t("providers.snaptrade.oauth_status_ready", default: "Use a device code to authorize Sure for SnapTrade.") %> + <% end %> +

+
+
- <%= form.text_field :consumer_key, - label: t("providers.snaptrade.consumer_key_label"), - placeholder: is_new_record ? t("providers.snaptrade.consumer_key_placeholder") : t("providers.snaptrade.consumer_key_update_placeholder"), - type: :password %> + <%= render DS::Link.new( + text: oauth_button_text, + icon: "external-link", + variant: :primary, + full_width: true, + href: oauth_href, + data: { turbo_frame: "drawer" } + ) %> +
-
- <%= form.submit is_new_record ? t("providers.snaptrade.save_button") : t("providers.snaptrade.update_button") %> + <%= render DS::Disclosure.new(variant: :card_inset) do |disclosure| %> + <% disclosure.with_summary_content do %> +
+
+

+ <%= t("providers.snaptrade.legacy_credentials_title", default: "Use legacy API credentials") %> +

+

+ <%= t("providers.snaptrade.legacy_credentials_description", default: "Use Client ID and Consumer Key setup if your SnapTrade account has not enabled OAuth.") %> +

+
+ <%= icon( + "chevron-down", + class: "mt-0.5 text-secondary group-open:rotate-180 motion-safe:transition-transform motion-safe:duration-150" + ) %> +
+ <% end %> + +
+ <%= render "settings/providers/setup_steps", + steps: [ + t("providers.snaptrade.step_1_html").html_safe, + t("providers.snaptrade.step_2"), + t("providers.snaptrade.step_3"), + t("providers.snaptrade.step_4") + ] %> + + <%= styled_form_with model: snaptrade_item, + url: is_new_record ? snaptrade_items_path : snaptrade_item_path(snaptrade_item), + scope: :snaptrade_item, + method: is_new_record ? :post : :patch, + data: { turbo: true }, + class: "space-y-3" do |form| %> + <%= form.text_field :client_id, + label: t("providers.snaptrade.client_id_label"), + placeholder: is_new_record ? + t("providers.snaptrade.client_id_placeholder") : + t("providers.snaptrade.client_id_update_placeholder"), + type: :password %> + + <%= form.text_field :consumer_key, + label: t("providers.snaptrade.consumer_key_label"), + placeholder: is_new_record ? + t("providers.snaptrade.consumer_key_placeholder") : + t("providers.snaptrade.consumer_key_update_placeholder"), + type: :password %> + +
+ <%= form.submit is_new_record ? t("providers.snaptrade.save_button") : t("providers.snaptrade.update_button") %> +
+ <% end %>
<% end %> - <% items = local_assigns[:snaptrade_items] || @snaptrade_items || Current.family.snaptrade_items.where.not(client_id: [nil, ""]) %> - - <% if items&.any? %> - <% item = items.first %> - <% unless item.user_registered? %> + <% if snaptrade_item.persisted? %> + <% unless snaptrade_item.user_registered? %>

<%= t("providers.snaptrade.status_needs_registration") %>

@@ -54,15 +118,14 @@ <% end %> <% end %> - <% if items&.any? && items.first.user_registered? %> - <% item = items.first %> + <% if snaptrade_item.persisted? && snaptrade_item.user_registered? %>
<%= render DS::Disclosure.new( variant: :inline, data: { controller: "lazy-load", action: "toggle->lazy-load#toggled", - lazy_load_url_value: connections_snaptrade_item_path(item), + lazy_load_url_value: connections_snaptrade_item_path(snaptrade_item), lazy_load_auto_open_param_value: "manage" } ) do |disclosure| %> @@ -70,9 +133,9 @@

- <%= t("providers.snaptrade.status_connected", count: item.snaptrade_accounts.count) %> - <% if item.unlinked_accounts_count > 0 %> - (<%= t("providers.snaptrade.needs_setup", count: item.unlinked_accounts_count) %>) + <%= t("providers.snaptrade.status_connected", count: snaptrade_item.snaptrade_accounts.count) %> + <% if snaptrade_item.unlinked_accounts_count > 0 %> + (<%= t("providers.snaptrade.needs_setup", count: snaptrade_item.unlinked_accounts_count) %>) <% end %>

diff --git a/app/views/snaptrade_items/_snaptrade_item.html.erb b/app/views/snaptrade_items/_snaptrade_item.html.erb index 4314200c8..9ae436a5b 100644 --- a/app/views/snaptrade_items/_snaptrade_item.html.erb +++ b/app/views/snaptrade_items/_snaptrade_item.html.erb @@ -2,6 +2,9 @@ <%= tag.div id: dom_id(snaptrade_item) do %> <% unlinked_count = snaptrade_item.unlinked_accounts_count %> + <% snaptrade_registered = snaptrade_item.user_registered? %> + <% brokerage_connect_href = snaptrade_registered ? connect_snaptrade_item_path(snaptrade_item) : oauth_connect_snaptrade_items_path(item_id: snaptrade_item.id) %> + <% brokerage_connect_frame = snaptrade_registered ? "_top" : :drawer %> <%= render DS::Disclosure.new(variant: :card, open: true) do |disclosure| %> <% disclosure.with_summary_content do %> @@ -60,12 +63,13 @@ <% if Current.user&.admin? %>
- <% if snaptrade_item.requires_update? || !snaptrade_item.user_registered? %> + <% if snaptrade_item.requires_update? || !snaptrade_registered %> <%= render DS::Link.new( text: t(".reconnect"), icon: "link", variant: "secondary", - href: connect_snaptrade_item_path(snaptrade_item) + href: brokerage_connect_href, + frame: brokerage_connect_frame ) %> <% else %> <%= icon( @@ -81,7 +85,8 @@ variant: "link", text: t(".connect_brokerage"), icon: "plus", - href: connect_snaptrade_item_path(snaptrade_item) + href: brokerage_connect_href, + frame: brokerage_connect_frame ) %> <% if unlinked_count > 0 %> <% menu.with_item( @@ -150,7 +155,8 @@ text: t(".connect_brokerage"), icon: "link", variant: "primary", - href: connect_snaptrade_item_path(snaptrade_item) + href: brokerage_connect_href, + frame: brokerage_connect_frame ) %>
<% end %> diff --git a/app/views/snaptrade_items/oauth_device_flow.html.erb b/app/views/snaptrade_items/oauth_device_flow.html.erb new file mode 100644 index 000000000..dead92236 --- /dev/null +++ b/app/views/snaptrade_items/oauth_device_flow.html.erb @@ -0,0 +1,112 @@ +<%= render DS::Dialog.new(frame: "drawer", responsive: true, auto_open: true) do |dialog| %> + <% dialog.with_header(custom_header: true) do %> + <%= render "settings/providers/drawer_header", + provider_key: "snaptrade", + title: t("snaptrade_items.oauth_device_flow.title", default: "Connect SnapTrade") %> + <% end %> + + <% dialog.with_body do %> +
+

+ <%= t("snaptrade_items.oauth_device_flow.subtitle", default: "Authorize Sure from SnapTrade") %> +

+ + <% if @error_message.present? %> + <%= render DS::Alert.new(message: @error_message, variant: :error) %> + <% end %> + + <% if @device_authorization.present? %> +
+

+ <%= t( + "snaptrade_items.oauth_device_flow.instructions", + default: "Open SnapTrade and confirm this device code, then return here to complete authorization." + ) %> +

+ +
+

+ <%= t("snaptrade_items.oauth_device_flow.code_label", default: "Device code") %> +

+

<%= @device_authorization["user_code"] %>

+
+ + <%= render DS::Link.new( + text: t("snaptrade_items.oauth_device_flow.open_snaptrade", default: "Open SnapTrade"), + href: @device_authorization["verification_uri_complete"].presence || @device_authorization["verification_uri"], + variant: :primary, + icon: "external-link", + target: "_blank", + rel: "noopener noreferrer", + full_width: true + ) %> +
+ + <%= form_with url: complete_oauth_device_flow_snaptrade_item_path(@snaptrade_item), + method: :post, + data: { turbo_frame: "drawer" }, + class: "space-y-3" do %> + <%= hidden_field_tag :device_code, @device_authorization["device_code"] %> + <%= hidden_field_tag :user_code, @device_authorization["user_code"] %> + <%= hidden_field_tag :verification_uri, @device_authorization["verification_uri"] %> + <%= hidden_field_tag :verification_uri_complete, @device_authorization["verification_uri_complete"] %> + <%= hidden_field_tag :expires_in, @device_authorization["expires_in"] %> + <%= hidden_field_tag :interval, @device_authorization["interval"] %> + <%= hidden_field_tag :return_to, @return_to if @return_to.present? %> + <%= hidden_field_tag :accountable_type, @accountable_type if @accountable_type.present? %> + +
+ <%= render DS::Link.new( + text: t("snaptrade_items.oauth_device_flow.cancel_button", default: "Cancel"), + variant: :secondary, + href: connect_form_settings_providers_path(provider_key: "snaptrade"), + data: { turbo_frame: "drawer" } + ) %> + <%= render DS::Button.new( + text: t("snaptrade_items.oauth_device_flow.complete_button", default: "I've authorized SnapTrade"), + variant: :primary, + icon: "check", + type: :submit + ) %> +
+ <% end %> + <% else %> + <% if @error_message.blank? %> + <%= form_with url: start_oauth_connect_snaptrade_items_path, + method: :post, + data: { turbo_frame: "drawer" }, + class: "space-y-3" do %> + <%= hidden_field_tag :item_id, @snaptrade_item.id if @snaptrade_item&.persisted? %> + <%= hidden_field_tag :scope, @oauth_scope if @oauth_scope.present? %> + <%= hidden_field_tag :return_to, @return_to if @return_to.present? %> + <%= hidden_field_tag :accountable_type, @accountable_type if @accountable_type.present? %> + +
+ <%= render DS::Link.new( + text: t("snaptrade_items.oauth_device_flow.cancel_button", default: "Cancel"), + variant: :secondary, + href: connect_form_settings_providers_path(provider_key: "snaptrade"), + data: { turbo_frame: "drawer" } + ) %> + <%= render DS::Button.new( + text: t("snaptrade_items.oauth_device_flow.start_button", default: "Start authorization"), + variant: :primary, + icon: "external-link", + type: :submit + ) %> +
+ <% end %> + <% else %> +
+ <%= render DS::Link.new( + text: t("snaptrade_items.oauth_device_flow.cancel_button", default: "Cancel"), + variant: :secondary, + href: connect_form_settings_providers_path(provider_key: "snaptrade"), + data: { turbo_frame: "drawer" } + ) %> +
+ <% end %> + <% end %> +
+ <% end %> +<% end %> diff --git a/app/views/snaptrade_items/setup_accounts.html.erb b/app/views/snaptrade_items/setup_accounts.html.erb index 363626feb..7bacde6f7 100644 --- a/app/views/snaptrade_items/setup_accounts.html.erb +++ b/app/views/snaptrade_items/setup_accounts.html.erb @@ -26,7 +26,7 @@

<%= icon "alert-triangle", size: "xs", class: "inline-block mr-1" %> - <%= t("snaptrade_items.setup_accounts.free_tier_note", default: "SnapTrade free tier allows 5 brokerage connections. Check your SnapTrade dashboard for current usage.") %> + <%= t("snaptrade_items.setup_accounts.free_tier_note", default: "SnapTrade free tier allows 20 brokerage connections.") %>

@@ -70,11 +70,23 @@

+ <% + brokerage_connect_href = if @snaptrade_item.user_registered? + connect_snaptrade_item_path(@snaptrade_item) + else + oauth_connect_snaptrade_items_path( + item_id: @snaptrade_item.id, + accountable_type: params[:accountable_type], + return_to: params[:return_to] + ) + end + brokerage_connect_frame = @snaptrade_item.user_registered? ? "_top" : "drawer" + %> <%= render DS::Link.new( text: t("snaptrade_items.setup_accounts.try_again", default: "Connect Brokerage"), variant: "primary", - href: connect_snaptrade_item_path(@snaptrade_item), - frame: "_top" + href: brokerage_connect_href, + frame: brokerage_connect_frame ) %> <%= render DS::Link.new( text: t("snaptrade_items.setup_accounts.back_to_settings", default: "Back to Settings"), diff --git a/config/locales/views/snaptrade_items/en.yml b/config/locales/views/snaptrade_items/en.yml index e601741cc..d176d71f9 100644 --- a/config/locales/views/snaptrade_items/en.yml +++ b/config/locales/views/snaptrade_items/en.yml @@ -28,6 +28,22 @@ en: not_configured: "SnapTrade is not configured." select_accounts: not_configured: "SnapTrade is not configured." + oauth_device_flow: + title: "Connect SnapTrade" + subtitle: "Authorize Sure for SnapTrade" + instructions: "Open SnapTrade and confirm this device code, then return here to complete authorization." + code_label: "Device code" + open_snaptrade: "Open SnapTrade" + start_button: "Start authorization" + complete_button: "I've authorized SnapTrade" + cancel_button: "Cancel" + missing_client_id: "SnapTrade OAuth client ID is not configured. Add SNAPTRADE_OAUTH_CLIENT_ID to .env.local, restart the app, then try again." + complete_oauth_device_flow: + success: "SnapTrade authorization complete." + setup_incomplete: "SnapTrade authorization is complete, but API credentials are required before accounts can sync." + failed: "Unable to complete SnapTrade OAuth device authorization. Please try again." + start_oauth_device_flow: + failed: "Unable to start SnapTrade OAuth device authorization. Please try again." select_existing_account: not_found: "Account or SnapTrade configuration not found." title: "Link to SnapTrade Account" @@ -67,7 +83,7 @@ en: info_cost_basis: "Cost basis per position (when available)" info_activities: "Trade history with activity labels (Buy, Sell, Dividend, etc.)" info_history: "Up to 3 years of transaction history" - free_tier_note: "SnapTrade free tier allows 5 brokerage connections. Check your SnapTrade dashboard for current usage." + free_tier_note: "SnapTrade free tier allows 20 brokerage connections." no_accounts_title: "No Accounts Found" no_accounts_message: "No brokerage accounts were found. This can happen if you cancelled the connection or if your brokerage isn't supported." try_again: "Connect Brokerage" @@ -121,7 +137,14 @@ en: step_2: "Copy your Client ID and Consumer Key from the dashboard" step_3: "Enter your credentials below and click Save" step_4: "Go to the Accounts page and use 'Connect another brokerage' to link your investment accounts" - free_tier_warning: "SnapTrade's free tier covers 5 brokerage connections. Upgrade on SnapTrade for more." + free_tier_warning: "SnapTrade's free tier covers 20 brokerage connections." + oauth_title: "SnapTrade OAuth" + oauth_status_ready: "Use a device code to authorize Sure for SnapTrade." + oauth_status_authorized: "Authorized with SnapTrade." + oauth_connect_button: "Connect with SnapTrade" + oauth_reauthorize_button: "Reauthorize" + legacy_credentials_title: "Use legacy API credentials" + legacy_credentials_description: "Use Client ID and Consumer Key setup if your SnapTrade account has not enabled OAuth." client_id_label: "Client ID" client_id_placeholder: "Enter your SnapTrade Client ID" client_id_update_placeholder: "Enter new Client ID to update" diff --git a/config/routes.rb b/config/routes.rb index 7dc7b69a1..ec7c058e1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -105,6 +105,8 @@ Rails.application.routes.draw do get :select_existing_account post :link_existing_account get :callback + get :oauth_connect + post :start_oauth_connect end member do diff --git a/test/controllers/snaptrade_items_controller_test.rb b/test/controllers/snaptrade_items_controller_test.rb index a334bb800..9c8bcf212 100644 --- a/test/controllers/snaptrade_items_controller_test.rb +++ b/test/controllers/snaptrade_items_controller_test.rb @@ -60,7 +60,7 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest .stubs(:complete_oauth_device_flow!) .raises(error) - post complete_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { device_code: "device-code" } + post complete_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { device_code: "device-code" }, as: :json assert_response :bad_request payload = JSON.parse(response.body) @@ -80,7 +80,7 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest .stubs(:complete_oauth_device_flow!) .raises(error) - post complete_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { device_code: "device-code" } + post complete_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { device_code: "device-code" }, as: :json assert_response :bad_gateway payload = JSON.parse(response.body) @@ -92,7 +92,7 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest .stubs(:complete_oauth_device_flow!) .raises(Provider::Snaptrade::ConfigurationError.new("missing secret at /srv/app/config.yml")) - post complete_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { device_code: "device-code" } + post complete_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { device_code: "device-code" }, as: :json assert_response :unprocessable_entity payload = JSON.parse(response.body) @@ -111,6 +111,116 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest assert_equal "Unable to start SnapTrade OAuth device authorization. Please try again.", payload["error"] end + test "start oauth device flow falls back to read for invalid scope" do + SnaptradeItem.any_instance + .expects(:start_oauth_device_flow) + .with(scope: "read") + .returns("device_code" => "device-code") + + post start_oauth_device_flow_snaptrade_item_url(@snaptrade_item), params: { scope: "write" } + + assert_response :success + assert_equal "device-code", JSON.parse(response.body)["device_code"] + end + + test "oauth_connect renders start form without side effects" do + sign_out + sign_in @user = users(:empty) + @user.family.snaptrade_items.destroy_all + Provider::Snaptrade.stubs(:oauth_client_id_configured?).returns(true) + SnaptradeItem.any_instance + .expects(:start_oauth_device_flow) + .never + + assert_no_difference "SnaptradeItem.count" do + get oauth_connect_snaptrade_items_url + end + + assert_response :success + assert_match "turbo-frame id=\"drawer\"", response.body + assert_match "Start authorization", response.body + assert_no_match "Open SnapTrade", response.body + end + + test "start_oauth_connect renders device authorization instructions" do + Provider::Snaptrade.stubs(:oauth_client_id_configured?).returns(true) + SnaptradeItem.any_instance + .stubs(:start_oauth_device_flow) + .returns( + "device_code" => "device-code", + "user_code" => "ABCD-EFGH", + "verification_uri" => "https://dashboard.snaptrade.com/activate", + "verification_uri_complete" => "https://dashboard.snaptrade.com/activate?user_code=ABCD-EFGH", + "expires_in" => 600, + "interval" => 5 + ) + + post start_oauth_connect_snaptrade_items_url + + assert_response :success + assert_match "turbo-frame id=\"drawer\"", response.body + assert_match "ABCD-EFGH", response.body + assert_match "Open SnapTrade", response.body + end + + test "start_oauth_connect falls back to read for invalid scope" do + Provider::Snaptrade.stubs(:oauth_client_id_configured?).returns(true) + SnaptradeItem.any_instance + .expects(:start_oauth_device_flow) + .with(scope: "read") + .returns( + "device_code" => "device-code", + "user_code" => "ABCD-EFGH", + "verification_uri" => "https://dashboard.snaptrade.com/activate", + "expires_in" => 600, + "interval" => 5 + ) + + post start_oauth_connect_snaptrade_items_url, params: { scope: "write" } + + assert_response :success + assert_match "ABCD-EFGH", response.body + end + + test "oauth_connect explains missing oauth client id without creating item" do + sign_out + sign_in @user = users(:empty) + @user.family.snaptrade_items.destroy_all + Provider::Snaptrade.stubs(:oauth_client_id_configured?).returns(false) + + assert_no_difference "SnaptradeItem.count" do + get oauth_connect_snaptrade_items_url + end + + assert_response :unprocessable_entity + assert_match "SNAPTRADE_OAUTH_CLIENT_ID", response.body + end + + test "start_oauth_connect creates oauth-only item when snaptrade is not configured" do + sign_out + sign_in @user = users(:empty) + @user.family.snaptrade_items.destroy_all + Provider::Snaptrade.stubs(:oauth_client_id_configured?).returns(true) + + SnaptradeItem.any_instance + .stubs(:start_oauth_device_flow) + .returns( + "device_code" => "device-code", + "user_code" => "ABCD-EFGH", + "verification_uri" => "https://dashboard.snaptrade.com/activate", + "expires_in" => 600, + "interval" => 5 + ) + + assert_difference "SnaptradeItem.count", 1 do + post start_oauth_connect_snaptrade_items_url + end + + assert_response :success + assert_match "ABCD-EFGH", response.body + assert_not @user.family.snaptrade_items.reload.last.credentials_configured? + end + test "select_accounts redirects unregistered users into connect flow" do sign_out sign_in @user = users(:empty) @@ -118,22 +228,110 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest get select_accounts_snaptrade_items_url, params: { accountable_type: "Investment", return_to: "setup_accounts" } - assert_redirected_to connect_snaptrade_item_path(snaptrade_item) + assert_redirected_to oauth_connect_snaptrade_items_path( + item_id: snaptrade_item.id, + accountable_type: "Investment", + return_to: "setup_accounts" + ) end - test "callback resumes setup flow after first-time connect detour" do + test "complete oauth device flow registers credentialed items and routes to setup" do sign_out sign_in @user = users(:empty) snaptrade_item = snaptrade_items(:pending_registration_item) - assert_difference "Sync.count", 1 do - get select_accounts_snaptrade_items_url, params: { accountable_type: "Investment", return_to: "setup_accounts" } - assert_redirected_to connect_snaptrade_item_path(snaptrade_item) + SnaptradeItem.any_instance + .stubs(:complete_oauth_device_flow!) + .returns( + "token_type" => "Bearer", + "scope" => "read", + "expires_in" => 3600 + ) + SnaptradeItem.any_instance + .stubs(:user_registered?) + .returns(false, true) + SnaptradeItem.any_instance + .expects(:ensure_user_registered!) + .once + .returns(true) - get callback_snaptrade_items_url, params: { item_id: snaptrade_item.id } + assert_difference "Sync.count", 1 do + post complete_oauth_device_flow_snaptrade_item_url(snaptrade_item), params: { + device_code: "device-code", + accountable_type: "Investment", + return_to: "setup_accounts" + } end - assert_redirected_to setup_accounts_snaptrade_item_path(snaptrade_item, accountable_type: "Investment") + assert_redirected_to setup_accounts_snaptrade_item_path( + snaptrade_item, + accountable_type: "Investment", + return_to: "setup_accounts" + ) + assert_equal "SnapTrade authorization complete.", flash[:notice] + end + + test "complete oauth device flow streams top-level navigation from drawer frame" do + sign_out + sign_in @user = users(:empty) + snaptrade_item = snaptrade_items(:pending_registration_item) + + SnaptradeItem.any_instance + .stubs(:complete_oauth_device_flow!) + .returns( + "token_type" => "Bearer", + "scope" => "read", + "expires_in" => 3600 + ) + SnaptradeItem.any_instance + .stubs(:user_registered?) + .returns(false, true) + SnaptradeItem.any_instance + .expects(:ensure_user_registered!) + .once + .returns(true) + setup_path = setup_accounts_snaptrade_item_path( + snaptrade_item, + accountable_type: "Investment", + return_to: "setup_accounts" + ) + + assert_difference "Sync.count", 1 do + post complete_oauth_device_flow_snaptrade_item_url(snaptrade_item), params: { + device_code: "device-code", + accountable_type: "Investment", + return_to: "setup_accounts" + }, headers: { "Turbo-Frame" => "drawer" } + end + + assert_response :success + assert_equal "text/vnd.turbo-stream.html", response.media_type + assert_match %( "Bearer", + "scope" => "read", + "expires_in" => 3600 + ) + + assert_no_difference "Sync.count" do + post complete_oauth_device_flow_snaptrade_item_url(snaptrade_item), params: { + device_code: "device-code" + } + end + + assert_redirected_to settings_providers_path + assert_match(/API credentials are required/, flash[:alert]) end test "select_accounts redirects registered users to setup flow" do @@ -145,13 +343,11 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest test "preload_accounts redirects unregistered users into connect flow" do sign_out sign_in @user = users(:empty) - snaptrade_item = snaptrade_items(:pending_registration_item) - assert_no_difference "Sync.count" do get preload_accounts_snaptrade_items_url end - assert_redirected_to connect_snaptrade_item_path(snaptrade_item) + assert_redirected_to oauth_connect_snaptrade_items_path(item_id: snaptrade_items(:pending_registration_item).id) end test "preload_accounts redirects registered users to setup flow and queues sync" do @@ -314,6 +510,8 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_select ".no-accounts-found", count: 1, message: "Expected the no-accounts UI to be shown after a completed sync with zero accounts" assert_select "#snaptrade-sync-spinner", count: 0, message: "Expected the spinner to be hidden when there is no active sync" + assert_select "a[href=?]", connect_snaptrade_item_path(@snaptrade_item), text: /Connect Brokerage/ + assert_no_match oauth_connect_snaptrade_items_path(item_id: @snaptrade_item.id), response.body end test "setup_accounts does not re-queue a sync when a sync is already in progress" do diff --git a/test/models/snaptrade_item_test.rb b/test/models/snaptrade_item_test.rb index 6b31c9895..008f71ba8 100644 --- a/test/models/snaptrade_item_test.rb +++ b/test/models/snaptrade_item_test.rb @@ -11,18 +11,23 @@ class SnaptradeItemTest < ActiveSupport::TestCase assert_includes item.errors[:name], "can't be blank" end - test "validates presence of client_id on create" do + test "requires client_id when consumer_key is present" do item = SnaptradeItem.new(family: @family, name: "Test", consumer_key: "test") assert_not item.valid? assert_includes item.errors[:client_id], "can't be blank" end - test "validates presence of consumer_key on create" do + test "requires consumer_key when client_id is present" do item = SnaptradeItem.new(family: @family, name: "Test", client_id: "test") assert_not item.valid? assert_includes item.errors[:consumer_key], "can't be blank" end + test "allows oauth-only items without api credentials" do + item = SnaptradeItem.new(family: @family, name: "Test") + assert item.valid? + end + test "credentials_configured? returns true when credentials are set" do item = SnaptradeItem.new( family: @family,