Files
sure/test/controllers/settings/providers_controller_test.rb
T
Juan José MataandClaude Opus 5 d6462f5fc9 feat(snaptrade): add device-flow OAuth alongside the browser redirect (#3126)
* feat(snaptrade): add device-flow OAuth alongside the browser redirect

SnapTrade could only be connected through the authorization-code + PKCE
flow, which needs a confidential OAuth client: SNAPTRADE_OAUTH_CLIENT_SECRET
and a redirect URI registered on the OAuth app. A deployment that cannot
register one had no path at all.

Add the device grant (RFC 8628) as a second way to obtain the same token,
so people can pick the flow that suits their deployment. Both grants end at
SnaptradeItem#apply_oauth_tokens!, so a device-authorized item is
indistinguishable from a redirect-authorized one from there on -- same
Bearer data calls, refresh, revocation and sync. Nothing about existing
authorized items changes: no schema change, no migration, and the PKCE path
is untouched.

- Provider::Snaptrade gains start_device_authorization and poll_device_token,
  with endpoints read from SnapTrade's OAuth metadata document (cached).
- oauth_configured? now means "some flow is available" (public client id),
  which is what gates syncing and the provider panel; the new
  authorization_code_configured? gates the redirect flow specifically.
- Token and revocation requests authenticate as a public client when no
  secret is configured -- client_id in the body instead of HTTP Basic.
  Without this a device-authorized item would authorize fine and then fail
  at its first token rotation.
- The settings panel offers both when both are available; every other entry
  point picks one through SnaptradeItemsHelper#snaptrade_authorize_path.
- The device page carries a failed attempt's code back into the form, so
  "not confirmed yet" is a retry rather than a restart.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

* fix(snaptrade): keep the provider panel's setup-step keys and cover both flows

Two test_unit failures from the panel change.

The setup steps were reordered and their keys renamed, which orphaned the
translations twelve locales already had for them and broke the test asserting
`oauth_setup_step_3`. The rename bought nothing: reword the steps in place
instead, leaving the callback URL on step 2 where the interpolation lives.

The panel tests stubbed `oauth_configured?`, which no longer decides which
buttons render -- that is now `authorization_code_configured?`. Stub both, so
the "configured" cases test the deployment they name, and add the device-only
case that was previously unreachable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

* fix(snaptrade): address device-flow review findings

Two real bugs from the bot reviews, plus consistency work.

The completion form posts into the `drawer` frame so errors re-render in place,
but a successful redirect was then followed as a frame navigation. Both
destinations carry the layout's empty `drawer` frame, so Turbo swapped that in
and merely closed the dialog: the notice was lost and `return_to=setup_accounts`
never advanced. Success now breaks out with a redirect stream action, the same
mechanism holdings and categorizes already use, while errors keep rendering in
the drawer.

RFC 8628 §3.1 requires a confidential client to authenticate its device
authorization request, and the panel offers the device code on deployments that
configured a secret. That request now carries the same client authentication as
the token request.

Token endpoint resolution is now shared by all three grants, since whatever
issued a token has to be what refreshes it. It reads the discovery document only
when already cached and never fetches it, so the browser flow keeps working off
the constant it has always used -- no new network call on refresh and no new way
for an existing authorized item to fail.

Also: the drawer no longer asks the provider whether it is configured, the
controller tells it; and the test helpers restore the previous OAuth config
rather than clearing it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

* fix(snaptrade): reject a device authorization response that cannot drive the flow

A 2xx missing device_code, user_code or a verification URI was passed straight
to the drawer, which then rendered a blank code and a link to nowhere -- a dead
end the user could only abandon. Every one of those fields is load-bearing, and
a response without them is partial or schema-changed, so fail with a message
instead. Same reasoning as the results-array check in get_positions.

verification_uri_complete substitutes for verification_uri when present, since
the drawer prefers it for the link anyway.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

* fix(snaptrade): filter device-flow codes from request logs

complete_oauth_device_flow receives the device code as a request parameter,
and none of the existing filter_parameters patterns is a substring of
"device_code" -- ParameterFilter matches on substrings, and "token", "_key",
"secret", "code_verifier" and "code_challenge" all miss it. So Rails' default
"Processing by ... Parameters: {...}" line was writing it in plaintext.

That matters more here than ordinary log hygiene: the device code is the only
capability check on redemption. Unlike the redirect flow's state, nothing binds
a device code to the family that requested it, so anyone who can read the logs
could redeem another family's in-flight authorization into their own item and
pick up a token for that family's brokerage data.

Adds :device_code, :user_code and :verification_uri_complete (which embeds the
user code) to the filter list, with a regression test in the style of the
existing Sophtron credential-filtering test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

* fix(snaptrade): bind a pending device authorization to its session

The device code was posted back from the drawer as a form field, so the
request body was the only thing deciding which item a pending authorization
redeemed into. Nothing tied a code to the family that asked for it -- the
guarantee `state` gives the redirect flow -- so a code recovered from
anywhere could be redeemed into an item belonging to someone else, handing
them a token for the victim's brokerage data.

Hold the pending authorization in the session instead, where oauth_callback
already keeps its code_verifier and state:

- start_oauth_device_flow records the code, what the page displays, the
  family, the item and the return_to context under :snaptrade_device_flow.
- complete_oauth_device_flow reads the code from there and refuses unless
  the flow was started by this session for this family and this item. A
  device_code parameter is no longer read at all, so there is no longer a
  way to inject one.
- return_to and accountable_type come from the session too, so completion
  needs nothing from the form to find its way back.

The code now never reaches the browser, which also makes the previous
commit's log filtering a second line of defence rather than the only one.

A failed attempt keeps the code only while it is still redeemable: expired_token
and access_denied clear it so the page offers a fresh start, while a transient
failure leaves it in place to retry. expires_in and interval are no longer
carried anywhere, since nothing ever read them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

* fix(snaptrade): use one token endpoint for every grant

poll_device_token resolved the token endpoint from the cached discovery
document while exchange_code and refresh_tokens used TOKEN_URL, so which
URL a device-issued token was refreshed at depended on whether the 12h
metadata cache was still warm. If the discovered endpoint ever differed
from the constant, a device-authorized item would work until the cache
lapsed and then fail its first rotation -- and fail invisibly, since a
refresh failure marks the connection requires_update.

Resolve it by removing the choice rather than by making refresh depend on
discovery. RFC 8628 §3.4 redeems a device code at the authorization
server's token endpoint, the same one the authorization code grant uses:
there is one token endpoint, not one per grant, and nothing to keep in
sync between issuing a token and refreshing it. TOKEN_URL is also the
endpoint the browser flow has been using in production, so it is the one
with evidence behind it. Discovery is still consulted, but only for
device_authorization_endpoint, which has no hardcoded equivalent.

This also keeps refresh free of any network dependency it did not already
have: reintroducing discovery there would have put a fetch, with retries
and backoff, in front of every token rotation on items that never needed
one.

Also restore the previous OAuth configuration in the missing-client-id
test instead of leaving the client id nil, which made it order-dependent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-08-25 04:28:25 +02:00

533 lines
19 KiB
Ruby

require "test_helper"
class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest
include ActiveJob::TestHelper
setup do
ensure_tailwind_build
sign_in users(:family_admin)
# Ensure provider adapters are loaded for all tests
Provider::Factory.ensure_adapters_loaded
end
test "GET /settings/bank_sync redirects permanently to /settings/providers" do
get "/settings/bank_sync"
assert_redirected_to "/settings/providers"
assert_equal 301, response.status
end
test "can access when self hosting is disabled (managed mode)" do
Rails.configuration.stubs(:app_mode).returns("managed".inquiry)
get settings_providers_url
assert_response :success
patch settings_providers_url, params: { setting: { plaid_client_id: "test123" } }
assert_redirected_to settings_providers_url
end
test "should get show when self hosting is enabled" do
with_self_hosting do
get settings_providers_url
assert_response :success
end
end
test "shows configured Brex connections in bank sync settings" do
get settings_providers_url
assert_response :success
assert_includes response.body, "Brex"
assert_includes response.body, "Test Brex Connection"
assert_includes response.body, "brex-providers-panel"
end
test "shows Brex as available when family has no Brex connections" do
sign_in users(:empty)
get settings_providers_url
assert_response :success
assert_includes response.body, "Brex"
assert_includes response.body, I18n.t("settings.providers.taglines.brex")
assert_includes response.body, connect_form_settings_providers_path(provider_key: "brex")
refute_includes response.body, "Test Brex Connection"
end
test "sync all control submits with POST" do
SimplefinItem.create!(
family: families(:dylan_family),
name: "Test SimpleFIN Sync All Control",
access_url: "https://bridge.simplefin.org/simplefin/access"
)
with_self_hosting do
get settings_providers_url
assert_response :success
assert_select "form[action=?][method=?]", sync_all_settings_providers_path, "post"
end
end
test "correctly identifies declared vs dynamic fields" do
# All current provider fields are dynamic, but the logic should correctly
# distinguish between declared and dynamic fields
with_self_hosting do
# plaid_client_id is a dynamic field (not defined in Setting)
refute Setting.singleton_class.method_defined?(:plaid_client_id=),
"plaid_client_id= should NOT be defined on Setting's singleton class"
# openai_model IS a declared field (defined in Setting)
# but it's not a provider field, so it won't go through this controller
assert Setting.singleton_class.method_defined?(:openai_model=),
"openai_model= should be defined on Setting's singleton class"
end
end
test "updates dynamic provider fields using batch update" do
# plaid_client_id is a dynamic field, stored as an individual entry
with_self_hosting do
# Clear any existing plaid settings
Setting["plaid_client_id"] = nil
patch settings_providers_url, params: {
setting: { plaid_client_id: "test_client_id" }
}
assert_redirected_to settings_providers_url
assert_equal "test_client_id", Setting["plaid_client_id"]
end
end
test "batches multiple dynamic fields from same provider atomically" do
# Test that multiple fields from Plaid are updated as individual entries
with_self_hosting do
# Clear existing fields
Setting["plaid_client_id"] = nil
Setting["plaid_secret"] = nil
Setting["plaid_environment"] = nil
patch settings_providers_url, params: {
setting: {
plaid_client_id: "new_client_id",
plaid_secret: "new_secret",
plaid_environment: "production"
}
}
assert_redirected_to settings_providers_url
# All three should be present as individual entries
assert_equal "new_client_id", Setting["plaid_client_id"]
assert_equal "new_secret", Setting["plaid_secret"]
assert_equal "production", Setting["plaid_environment"]
end
end
test "batches dynamic fields from multiple providers atomically" do
# Test that fields from different providers are stored as individual entries
with_self_hosting do
# Clear existing fields
Setting["plaid_client_id"] = nil
Setting["plaid_secret"] = nil
Setting["plaid_eu_client_id"] = nil
Setting["plaid_eu_secret"] = nil
patch settings_providers_url, params: {
setting: {
plaid_client_id: "plaid_client",
plaid_secret: "plaid_secret",
plaid_eu_client_id: "plaid_eu_client",
plaid_eu_secret: "plaid_eu_secret"
}
}
assert_redirected_to settings_providers_url
# All fields should be present
assert_equal "plaid_client", Setting["plaid_client_id"]
assert_equal "plaid_secret", Setting["plaid_secret"]
assert_equal "plaid_eu_client", Setting["plaid_eu_client_id"]
assert_equal "plaid_eu_secret", Setting["plaid_eu_secret"]
end
end
test "preserves existing dynamic fields when updating new ones" do
# Test that updating some fields doesn't overwrite other existing fields
with_self_hosting do
# Set initial fields
Setting["existing_field_1"] = "value1"
Setting["plaid_client_id"] = "old_client_id"
# Update one field and add a new one
patch settings_providers_url, params: {
setting: {
plaid_client_id: "new_client_id",
plaid_secret: "new_secret"
}
}
assert_redirected_to settings_providers_url
# Existing unrelated field should still be there
assert_equal "value1", Setting["existing_field_1"]
# Updated field should have new value
assert_equal "new_client_id", Setting["plaid_client_id"]
# New field should be added
assert_equal "new_secret", Setting["plaid_secret"]
end
end
test "skips placeholder values for secret fields" do
with_self_hosting do
# Set an initial secret value
Setting["plaid_secret"] = "real_secret"
# Try to update with placeholder
patch settings_providers_url, params: {
setting: {
plaid_client_id: "new_client_id",
plaid_secret: "********" # Placeholder value
}
}
assert_redirected_to settings_providers_url
# Client ID should be updated
assert_equal "new_client_id", Setting["plaid_client_id"]
# Secret should remain unchanged
assert_equal "real_secret", Setting["plaid_secret"]
end
end
test "converts blank values to nil and removes from dynamic_fields" do
with_self_hosting do
# Set initial values
Setting["plaid_client_id"] = "old_value"
assert_equal "old_value", Setting["plaid_client_id"]
assert Setting.key?("plaid_client_id")
patch settings_providers_url, params: {
setting: { plaid_client_id: " " } # Blank string with spaces
}
assert_redirected_to settings_providers_url
assert_nil Setting["plaid_client_id"]
# Entry should be removed, not just set to nil
refute Setting.key?("plaid_client_id"),
"nil values should delete the entry"
end
end
test "handles sequential updates to different dynamic fields safely" do
# This test simulates what would happen if two requests tried to update
# different dynamic fields sequentially. With individual entries,
# all changes should be preserved without conflicts.
with_self_hosting do
Setting["existing_field"] = "existing_value"
# Simulate first request updating plaid fields
patch settings_providers_url, params: {
setting: {
plaid_client_id: "client_id_1",
plaid_secret: "secret_1"
}
}
# Existing field should still be there
assert_equal "existing_value", Setting["existing_field"]
# New fields should be added
assert_equal "client_id_1", Setting["plaid_client_id"]
assert_equal "secret_1", Setting["plaid_secret"]
# Simulate second request updating different plaid fields
patch settings_providers_url, params: {
setting: {
plaid_environment: "production"
}
}
# All previously set fields should still be there
assert_equal "existing_value", Setting["existing_field"]
assert_equal "client_id_1", Setting["plaid_client_id"]
assert_equal "secret_1", Setting["plaid_secret"]
assert_equal "production", Setting["plaid_environment"]
end
end
test "only processes valid configuration fields" do
with_self_hosting do
# Try to update a field that doesn't exist in any provider configuration
patch settings_providers_url, params: {
setting: {
plaid_client_id: "valid_field",
fake_field_that_does_not_exist: "should_be_ignored"
}
}
assert_redirected_to settings_providers_url
# Valid field should be updated
assert_equal "valid_field", Setting["plaid_client_id"]
# Invalid field should not be stored
assert_nil Setting["fake_field_that_does_not_exist"]
end
end
test "calls reload_configuration on updated providers" do
with_self_hosting do
# Mock the adapter class to verify reload_configuration is called
Provider::PlaidAdapter.expects(:reload_configuration).once
patch settings_providers_url, params: {
setting: { plaid_client_id: "new_client_id" }
}
assert_redirected_to settings_providers_url
end
end
test "reloads configuration for multiple providers when updated" do
with_self_hosting do
# Both Plaid providers (US and EU) should have their configuration reloaded
Provider::PlaidAdapter.expects(:reload_configuration).once
Provider::PlaidEuAdapter.expects(:reload_configuration).once
patch settings_providers_url, params: {
setting: {
plaid_client_id: "plaid_client",
plaid_eu_client_id: "plaid_eu_client"
}
}
assert_redirected_to settings_providers_url
end
end
test "logs errors when update fails" do
with_self_hosting do
# Test that errors during update are properly logged and handled gracefully
# We'll force an error by making the []= method raise
Setting.expects(:[]=).with("plaid_client_id", "test").raises(StandardError.new("Database error")).once
# Mock logger to verify error is logged (pin both the exception class
# name and the message so a regression that drops one still fails).
Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings: StandardError - Database error/)).once
patch settings_providers_url, params: {
setting: { plaid_client_id: "test" }
}
# Controller should handle the error gracefully with generic message (no internal details)
assert_response :unprocessable_entity
assert_equal "Failed to update provider settings. Please try again.", flash[:alert]
end
end
test "shows no changes message when no fields are updated" do
with_self_hosting do
# Only send a secret field with placeholder value (which gets skipped)
Setting["plaid_secret"] = "existing_secret"
patch settings_providers_url, params: {
setting: { plaid_secret: "********" }
}
assert_redirected_to settings_providers_url
assert_equal "No changes were made", flash[:notice]
end
end
test "POST sync_all enqueues SyncAllProvidersJob" do
SimplefinItem.create!(
family: families(:dylan_family),
name: "Test SimpleFIN Sync All",
access_url: "https://bridge.simplefin.org/simplefin/access"
)
families(:dylan_family).update_column(:last_sync_all_attempted_at, nil)
assert_enqueued_with(job: SyncAllProvidersJob) do
post sync_all_settings_providers_path
end
assert_redirected_to settings_providers_path
follow_redirect!
assert_response :success
assert_match(/Syncing all connected providers/i, response.body)
end
test "POST sync_all respects recent sync throttle" do
families(:dylan_family).update_column(:last_sync_all_attempted_at, Time.current)
assert_no_enqueued_jobs only: SyncAllProvidersJob do
post sync_all_settings_providers_path
end
assert_redirected_to settings_providers_path
assert_equal I18n.t("settings.providers.sync_all_recently"), flash[:notice]
end
test "POST sync for simplefin without an active Simplefin sync enqueues SyncJob" do
item = SimplefinItem.create!(
family: families(:dylan_family),
name: "Test SimpleFIN Per Row Sync",
access_url: "https://bridge.simplefin.org/simplefin/access"
)
Sync.where(syncable_type: "SimplefinItem", syncable_id: item.id).delete_all
assert_enqueued_jobs 1, only: SyncJob do
post sync_provider_settings_providers_path(provider_key: "simplefin")
end
assert_redirected_to settings_providers_path
follow_redirect!
assert_response :success
assert_match(/Sync started/i, response.body)
end
test "POST sync for brex without an active Brex sync enqueues SyncJob" do
item = brex_items(:one)
Sync.where(syncable_type: "BrexItem", syncable_id: item.id).delete_all
assert_enqueued_jobs 1, only: SyncJob do
post sync_provider_settings_providers_path(provider_key: "brex")
end
assert_redirected_to settings_providers_path
follow_redirect!
assert_response :success
assert_match(/Sync started/i, response.body)
end
test "GET show includes Interactive Brokers in bank sync providers" do
get settings_providers_url
assert_response :success
assert_match(/Interactive Brokers/i, response.body)
assert_match(/Flex Query/i, response.body)
end
test "GET connect_form renders Interactive Brokers panel" do
get connect_form_settings_providers_path(provider_key: "ibkr")
assert_response :success
assert_match(/Interactive Brokers/i, response.body)
assert_match(/Query ID/i, response.body)
end
test "GET connect_form for snaptrade shows OAuth setup instructions when instance is not configured" do
Provider::Snaptrade.stubs(:oauth_configured?).returns(false)
get connect_form_settings_providers_path(provider_key: "snaptrade")
assert_response :success
assert_includes response.body, I18n.t("providers.snaptrade.oauth_setup_step_3")
refute_includes response.body, I18n.t("providers.snaptrade.oauth_connect_button")
refute_includes response.body, I18n.t("providers.snaptrade.oauth_status_ready")
end
test "GET connect_form for snaptrade shows connect CTA when configured but item is not authorized" do
sign_in users(:empty)
Provider::Snaptrade.stubs(:oauth_configured?).returns(true)
Provider::Snaptrade.stubs(:authorization_code_configured?).returns(true)
get connect_form_settings_providers_path(provider_key: "snaptrade")
assert_response :success
assert_includes response.body, I18n.t("providers.snaptrade.oauth_connect_button")
assert_includes response.body, I18n.t("providers.snaptrade.oauth_status_ready")
# Both grants are available here, so the device code is offered alongside.
assert_includes response.body, I18n.t("providers.snaptrade.oauth_device_button")
refute_includes response.body, I18n.t("providers.snaptrade.oauth_status_authorized")
refute_includes response.body, I18n.t("providers.snaptrade.oauth_reauthorize_button")
end
test "GET connect_form for snaptrade offers only the device code without a confidential client" do
sign_in users(:empty)
Provider::Snaptrade.stubs(:oauth_configured?).returns(true)
Provider::Snaptrade.stubs(:authorization_code_configured?).returns(false)
get connect_form_settings_providers_path(provider_key: "snaptrade")
assert_response :success
assert_includes response.body, I18n.t("providers.snaptrade.oauth_device_button")
assert_includes response.body, I18n.t("providers.snaptrade.oauth_status_ready_device")
# The browser redirect needs a client secret this deployment does not have.
refute_includes response.body, I18n.t("providers.snaptrade.oauth_connect_button")
end
test "GET connect_form for snaptrade shows authorized status and reauthorize CTA when item is connected" do
# Default signed-in user (family_admin) belongs to dylan_family, which owns
# the oauth-authorized `configured_item` fixture.
Provider::Snaptrade.stubs(:oauth_configured?).returns(true)
Provider::Snaptrade.stubs(:authorization_code_configured?).returns(true)
get connect_form_settings_providers_path(provider_key: "snaptrade")
assert_response :success
assert_includes response.body, I18n.t("providers.snaptrade.oauth_status_authorized")
assert_includes response.body, I18n.t("providers.snaptrade.oauth_reauthorize_button")
assert_includes response.body, I18n.t("providers.snaptrade.manage_connections")
refute_includes response.body, I18n.t("providers.snaptrade.oauth_connect_button")
end
test "POST sync for ibkr without an active Ibkr sync enqueues SyncJob" do
item = ibkr_items(:configured_item)
Sync.where(syncable_type: "IbkrItem", syncable_id: item.id).delete_all
assert_enqueued_jobs 1, only: SyncJob do
post sync_provider_settings_providers_path(provider_key: "ibkr")
end
assert_redirected_to settings_providers_path
follow_redirect!
assert_response :success
assert_match(/Sync started/i, response.body)
end
test "non-admin users cannot update providers" do
with_self_hosting do
sign_in users(:family_member)
patch settings_providers_url, params: {
setting: { plaid_client_id: "test" }
}
assert_redirected_to root_path
assert_equal "Not authorized", flash[:alert]
# Value should not have changed
assert_nil Setting["plaid_client_id"]
end
end
test "uses singleton_class method_defined to detect declared fields" do
with_self_hosting do
# This test verifies the difference between respond_to? and singleton_class.method_defined?
# openai_model is a declared field
assert Setting.singleton_class.method_defined?(:openai_model=),
"openai_model= should be defined on Setting's singleton class"
assert Setting.respond_to?(:openai_model=),
"respond_to? should return true for declared field"
# plaid_client_id is a dynamic field
refute Setting.singleton_class.method_defined?(:plaid_client_id=),
"plaid_client_id= should NOT be defined on Setting's singleton class"
refute Setting.respond_to?(:plaid_client_id=),
"respond_to? should return false for dynamic field"
# Both methods currently return the same result, but singleton_class.method_defined?
# is more explicit and reliable for checking if a method is actually defined
end
end
end