Skip to content

Commit 8fa4503

Browse files
committed
Fixes #TBFL - Prevent spurious calls to proxy when rendering api responses
The previous "return stored value or fetch a fresh one if missing" approach used for pubkey and ca_pubkey could cause calls to the proxy in unexpected situations. This could lead to problems like not being able to list smart proxies if the proxy which didn't have one of the pubkey fields filled was down. This change more strictly separates when values are only read and when values can be updated as part of reading.
1 parent 0588a71 commit 8fa4503

File tree

4 files changed

+93
-6
lines changed

4 files changed

+93
-6
lines changed

app/models/concerns/foreman_remote_execution/smart_proxy_extensions.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@ def self.prepended(base)
77
end
88
end
99

10-
def pubkey
11-
self[:pubkey] || update_pubkey
10+
def pubkey(refresh: true)
11+
self[:pubkey] || (refresh && update_pubkey || nil)
1212
end
1313

14-
def ca_pubkey
15-
self[:ca_pubkey] || update_ca_pubkey
14+
def ca_pubkey(refresh: true)
15+
self[:ca_pubkey] || (refresh && update_ca_pubkey || nil)
1616
end
1717

1818
def update_pubkey
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
attribute :ca_pubkey => :remote_execution_ca_pubkey
1+
node(:remote_execution_ca_pubkey) { |p| p.ca_pubkey(refresh: false) }
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
attribute :pubkey => :remote_execution_pubkey
1+
node(:remote_execution_pubkey) { |p| p.pubkey(refresh: false) }
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
require 'test_plugin_helper'
2+
3+
# rubocop:disable Rails/SkipsModelValidations
4+
class ForemanRemoteExecutionSmartProxyExtensionsTest < ActiveSupport::TestCase
5+
let(:proxy) { FactoryBot.create(:smart_proxy, :ssh) }
6+
7+
describe '#pubkey' do
8+
context 'when key is cached in the database' do
9+
it 'returns the cached key without fetching from proxy' do
10+
proxy.expects(:update_pubkey).never
11+
assert_equal 'ssh-rsa AAAAB3N...', proxy.pubkey
12+
assert_equal 'ssh-rsa AAAAB3N...', proxy.pubkey(refresh: false)
13+
end
14+
end
15+
16+
context 'when key is not cached' do
17+
before { proxy.update_column(:pubkey, nil) }
18+
19+
it 'fetches from proxy by default' do
20+
proxy.expects(:update_pubkey).returns('ssh-rsa FETCHED...')
21+
assert_equal 'ssh-rsa FETCHED...', proxy.pubkey
22+
end
23+
24+
it 'returns nil without fetching when refresh: false' do
25+
proxy.expects(:update_pubkey).never
26+
assert_nil proxy.pubkey(refresh: false)
27+
end
28+
end
29+
end
30+
31+
describe '#ca_pubkey' do
32+
context 'when key is cached in the database' do
33+
before { proxy.update_column(:ca_pubkey, 'ssh-rsa CA_KEY...') }
34+
35+
it 'returns the cached key without fetching from proxy' do
36+
proxy.expects(:update_ca_pubkey).never
37+
assert_equal 'ssh-rsa CA_KEY...', proxy.ca_pubkey
38+
assert_equal 'ssh-rsa CA_KEY...', proxy.ca_pubkey(refresh: false)
39+
end
40+
end
41+
42+
context 'when key is not cached' do
43+
before { proxy.update_column(:ca_pubkey, nil) }
44+
45+
it 'fetches from proxy by default' do
46+
proxy.expects(:update_ca_pubkey).returns('ssh-rsa FETCHED_CA...')
47+
assert_equal 'ssh-rsa FETCHED_CA...', proxy.ca_pubkey
48+
end
49+
50+
it 'returns nil without fetching when refresh: false' do
51+
proxy.expects(:update_ca_pubkey).never
52+
assert_nil proxy.ca_pubkey(refresh: false)
53+
end
54+
end
55+
end
56+
57+
describe '#refresh' do
58+
context 'when the key is cached' do
59+
before do
60+
proxy.update_column(:pubkey, 'ssh-rsa KEY...')
61+
proxy.update_column(:ca_pubkey, 'ssh-rsa CA_KEY...')
62+
end
63+
64+
it 'fetches the keys' do
65+
proxy.expects(:update_pubkey).once
66+
proxy.expects(:update_ca_pubkey).once
67+
68+
proxy.refresh
69+
end
70+
end
71+
72+
context 'when the key is not cached' do
73+
before do
74+
proxy.update_column(:pubkey, nil)
75+
proxy.update_column(:ca_pubkey, nil)
76+
end
77+
78+
it 'fetches the keys' do
79+
proxy.expects(:update_pubkey).once
80+
proxy.expects(:update_ca_pubkey).once
81+
82+
proxy.refresh
83+
end
84+
end
85+
end
86+
end
87+
# rubocop:enable Rails/SkipsModelValidations

0 commit comments

Comments
 (0)