mirror of
https://github.com/we-promise/sure.git
synced 2026-07-12 12:55:20 +00:00
Allow multiple active API keys per user (#2077)
* feat(api): allow multiple active API keys per user Previously a user could hold only one active API key; creating a new one silently revoked the existing key, breaking any app using it. Allow multiple named active keys instead. - Drop the one-active-key-per-source validation; add name uniqueness among the user's active visible keys (revoked names are reusable, same name allowed across users). - Rewrite Settings::ApiKeysController as a RESTful collection (index/show/new/create/destroy); create no longer revokes existing keys, and key lookup is scoped to the current user's active keys. - resource :api_key -> resources :api_keys. - Add an API keys list view with per-key revoke and an empty state; rewrite the show page for a single key. - Update i18n (remove single-key copy, pluralise nav label) and tests. * refactor(api): address review on multiple API keys - Remove unreachable destroy branches (cannot_revoke is guarded by the .visible 404; revoke! raises rather than returning false) and document that .visible is the demo-key revocation guard. - Delete the orphaned created.html.erb / created.turbo_stream.erb templates (no action renders them) and their unused locale keys. - Extract shared partials (_scope_badges, _status_indicator, _key_meta, _key_reveal, _usage) to de-duplicate the index and show views; unify the active-status indicator on the standard dot. - Carry forward the @container / @lg:flex-row / min-w-0 responsive fixes from #2079 into the shared key-reveal partial. * test(api): cover newly-created API key confirmation render * refactor(api): harden demo-key guard and address review nits - Document the demo-key revocation guard on ApiKey's `visible` scope (the authoritative spot) and add a model test locking the invariant that `.visible` excludes the demo monitoring key. - _scope_badges: use an i18n lookup with a humanize fallback instead of bypassing translation for unknown scopes. - _key_reveal: drop the hard-coded `id` from the shared partial; the system test now locates the key via its data-clipboard-target. * refactor(api-keys): migrate hand-rolled badges to DS::Pill Replace raw span elements in scope badges and status indicator with DS::Pill to align with the design system migration convention. * fix(loans): opening anchor now uses current balance, not original principal When creating a loan manually, the opening anchor valuation was being set to `initial_balance` (the original loan principal) instead of `account.balance` (the current outstanding balance). After the sync job ran, `account.balance` was overwritten to match the anchor, making every manually-created loan show its original principal as the current balance. Fix by always using `account.balance` for the opening anchor in `create_and_sync`, and reading `Loan#original_balance` from the `loans.initial_balance` column directly (with a fallback to `first_valuation_amount` for provider-synced loans that may not have the column populated). * fix(api-keys): strip accidental loan changes; rescue revoke! failures The fix(loans) commit was accidentally committed into this branch. Remove the loan-related changes from account.rb, loan.rb, and both test files, restoring them to their pre-loan-commit state. Also fix the destroy action: revoke! uses update! internally which raises ActiveRecord::RecordInvalid on failure rather than returning false. Add rescue for RecordInvalid and RecordNotDestroyed so failures produce a flash alert instead of a 500. Re-adds the revoke_failed locale key that was dropped from settings.api_keys.destroy. --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
This commit is contained in:
Will Wilson
@@ -7,29 +7,20 @@ class Settings::ApiKeysControllerTest < ActionDispatch::IntegrationTest
|
||||
sign_in @user
|
||||
end
|
||||
|
||||
test "should show no API key page when user has no active keys" do
|
||||
get settings_api_key_path
|
||||
assert_response :success
|
||||
end
|
||||
|
||||
test "should show current API key when user has active key" do
|
||||
@api_key = ApiKey.create!(
|
||||
test "index shows api keys list" do
|
||||
ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Test API Key",
|
||||
display_key: "test_key_123",
|
||||
name: "Listed Key",
|
||||
display_key: "listed_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
get settings_api_key_path
|
||||
get settings_api_keys_path
|
||||
assert_response :success
|
||||
assert_includes response.body, "Listed Key"
|
||||
end
|
||||
|
||||
test "should show new API key form" do
|
||||
get new_settings_api_key_path
|
||||
assert_response :success
|
||||
end
|
||||
|
||||
test "should redirect to show when user already has active key and tries to visit new" do
|
||||
test "new always renders form (no redirect when key exists)" do
|
||||
ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Existing API Key",
|
||||
@@ -38,57 +29,38 @@ class Settings::ApiKeysControllerTest < ActionDispatch::IntegrationTest
|
||||
)
|
||||
|
||||
get new_settings_api_key_path
|
||||
assert_redirected_to settings_api_key_path
|
||||
assert_response :success
|
||||
end
|
||||
|
||||
test "should create new API key with valid parameters" do
|
||||
test "create makes a new key without revoking existing keys" do
|
||||
existing = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Existing API Key",
|
||||
display_key: "existing_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
assert_difference "ApiKey.count", 1 do
|
||||
post settings_api_key_path, params: {
|
||||
post settings_api_keys_path, params: {
|
||||
api_key: {
|
||||
name: "Test Integration Key",
|
||||
name: "Brand New Key",
|
||||
scopes: "read_write"
|
||||
}
|
||||
}
|
||||
end
|
||||
|
||||
assert_redirected_to settings_api_key_path
|
||||
follow_redirect!
|
||||
assert_response :success
|
||||
new_key = @user.api_keys.active.visible.find_by(name: "Brand New Key")
|
||||
assert new_key.present?
|
||||
assert_redirected_to settings_api_key_path(new_key, newly_created: true)
|
||||
|
||||
api_key = @user.api_keys.active.first
|
||||
assert_equal "Test Integration Key", api_key.name
|
||||
assert_includes api_key.scopes, "read_write"
|
||||
existing.reload
|
||||
refute existing.revoked?
|
||||
assert_includes new_key.scopes, "read_write"
|
||||
end
|
||||
|
||||
test "should revoke existing key when creating new one" do
|
||||
old_key = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Old API Key",
|
||||
display_key: "old_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
post settings_api_key_path, params: {
|
||||
api_key: {
|
||||
name: "New API Key",
|
||||
scopes: "read_write"
|
||||
}
|
||||
}
|
||||
|
||||
assert_redirected_to settings_api_key_path
|
||||
follow_redirect!
|
||||
assert_response :success
|
||||
|
||||
old_key.reload
|
||||
assert old_key.revoked?
|
||||
|
||||
new_key = @user.api_keys.active.first
|
||||
assert_equal "New API Key", new_key.name
|
||||
end
|
||||
|
||||
test "should not create API key without name" do
|
||||
test "create rejects blank name" do
|
||||
assert_no_difference "ApiKey.count" do
|
||||
post settings_api_key_path, params: {
|
||||
post settings_api_keys_path, params: {
|
||||
api_key: {
|
||||
name: "",
|
||||
scopes: "read"
|
||||
@@ -99,93 +71,133 @@ class Settings::ApiKeysControllerTest < ActionDispatch::IntegrationTest
|
||||
assert_response :unprocessable_entity
|
||||
end
|
||||
|
||||
test "should not create API key without scopes" do
|
||||
# Ensure clean state for this specific test
|
||||
@user.api_keys.destroy_all
|
||||
initial_user_count = @user.api_keys.count
|
||||
|
||||
assert_no_difference "@user.api_keys.count" do
|
||||
post settings_api_key_path, params: {
|
||||
api_key: {
|
||||
name: "Test Key",
|
||||
scopes: []
|
||||
test "create rejects blank scopes" do
|
||||
assert_no_difference "ApiKey.count" do
|
||||
post settings_api_keys_path, params: {
|
||||
api_key: {
|
||||
name: "No Scopes Key",
|
||||
scopes: []
|
||||
}
|
||||
}
|
||||
}
|
||||
end
|
||||
|
||||
assert_response :unprocessable_entity
|
||||
end
|
||||
|
||||
assert_response :unprocessable_entity
|
||||
assert_equal initial_user_count, @user.api_keys.reload.count
|
||||
end
|
||||
test "create rejects duplicate active name" do
|
||||
ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Dup",
|
||||
display_key: "dup_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
test "should revoke API key" do
|
||||
@api_key = ApiKey.create!(
|
||||
assert_no_difference "ApiKey.count" do
|
||||
post settings_api_keys_path, params: {
|
||||
api_key: {
|
||||
name: "Dup",
|
||||
scopes: "read_write"
|
||||
}
|
||||
}
|
||||
end
|
||||
|
||||
assert_response :unprocessable_entity
|
||||
end
|
||||
|
||||
test "show renders a key" do
|
||||
created_key = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Test API Key",
|
||||
display_key: "test_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
delete settings_api_key_path
|
||||
|
||||
assert_redirected_to settings_api_key_path
|
||||
follow_redirect!
|
||||
get settings_api_key_path(created_key)
|
||||
assert_response :success
|
||||
|
||||
@api_key.reload
|
||||
assert @api_key.revoked?
|
||||
assert_includes response.body, "Test API Key"
|
||||
end
|
||||
|
||||
test "should handle revoke when no API key exists" do
|
||||
delete settings_api_key_path
|
||||
test "show renders the newly created confirmation" do
|
||||
created_key = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Fresh Key",
|
||||
display_key: "fresh_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
assert_redirected_to settings_api_key_path
|
||||
# Should not error even when no API key exists
|
||||
get settings_api_key_path(created_key, newly_created: true)
|
||||
assert_response :success
|
||||
assert_includes response.body, created_key.plain_key
|
||||
assert_select "h3", text: I18n.t("settings.api_keys.show.newly_created.heading")
|
||||
end
|
||||
|
||||
test "should only allow one active API key per user" do
|
||||
# Create first API key
|
||||
post settings_api_key_path, params: {
|
||||
api_key: {
|
||||
name: "First Key",
|
||||
scopes: "read"
|
||||
}
|
||||
}
|
||||
test "show 404s on another user's key" do
|
||||
other_user = users(:family_member)
|
||||
other_user.api_keys.destroy_all
|
||||
other_key = ApiKey.create!(
|
||||
user: other_user,
|
||||
name: "Other User Key",
|
||||
display_key: "other_user_key_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
first_key = @user.api_keys.active.first
|
||||
|
||||
# Create second API key
|
||||
post settings_api_key_path, params: {
|
||||
api_key: {
|
||||
name: "Second Key",
|
||||
scopes: "read_write"
|
||||
}
|
||||
}
|
||||
|
||||
# First key should be revoked
|
||||
first_key.reload
|
||||
assert first_key.revoked?
|
||||
|
||||
# Only one active key should exist
|
||||
assert_equal 1, @user.api_keys.active.count
|
||||
assert_equal "Second Key", @user.api_keys.active.first.name
|
||||
get settings_api_key_path(other_key)
|
||||
assert_response :not_found
|
||||
end
|
||||
|
||||
test "should generate secure random API key" do
|
||||
post settings_api_key_path, params: {
|
||||
test "destroy revokes the targeted key only" do
|
||||
key1 = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Key One",
|
||||
display_key: "key_one_123",
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
key2 = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Key Two",
|
||||
display_key: "key_two_123",
|
||||
scopes: [ "read_write" ]
|
||||
)
|
||||
|
||||
delete settings_api_key_path(key1)
|
||||
assert_redirected_to settings_api_keys_path
|
||||
|
||||
key1.reload
|
||||
key2.reload
|
||||
assert key1.revoked?
|
||||
refute key2.revoked?
|
||||
end
|
||||
|
||||
test "destroy cannot revoke demo monitoring key" do
|
||||
# set_api_key scopes to .visible which EXCLUDES the demo key, so the
|
||||
# demo key id is not found by the controller and the request 404s
|
||||
# before reaching the cannot_revoke branch.
|
||||
demo_key = ApiKey.create!(
|
||||
user: @user,
|
||||
name: "Demo Monitoring Key",
|
||||
display_key: ApiKey::DEMO_MONITORING_KEY,
|
||||
scopes: [ "read" ]
|
||||
)
|
||||
|
||||
delete settings_api_key_path(demo_key)
|
||||
assert_response :not_found
|
||||
|
||||
demo_key.reload
|
||||
refute demo_key.revoked?
|
||||
end
|
||||
|
||||
test "create generates a secure random API key" do
|
||||
post settings_api_keys_path, params: {
|
||||
api_key: {
|
||||
name: "Random Key Test",
|
||||
scopes: "read"
|
||||
}
|
||||
}
|
||||
|
||||
assert_redirected_to settings_api_key_path
|
||||
follow_redirect!
|
||||
assert_response :success
|
||||
|
||||
# Verify the API key was created with expected properties
|
||||
api_key = @user.api_keys.active.first
|
||||
assert api_key.present?
|
||||
assert_equal "Random Key Test", api_key.name
|
||||
assert_includes api_key.scopes, "read"
|
||||
created_key = @user.api_keys.active.visible.find_by(name: "Random Key Test")
|
||||
assert created_key.present?
|
||||
assert_redirected_to settings_api_key_path(created_key, newly_created: true)
|
||||
assert_includes created_key.scopes, "read"
|
||||
assert_equal 64, created_key.plain_key.length
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user