Skip to content

Commit

Permalink
Remove before_refresh from client construction in RefreshingCredentia…
Browse files Browse the repository at this point in the history
…ls (#2691)
  • Loading branch information
alextwoods authored Apr 21, 2022
1 parent 0ac3d0a commit f5c29fc
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 9 deletions.
2 changes: 2 additions & 0 deletions gems/aws-sdk-cognitoidentity/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Unreleased Changes
------------------

* Issue - Don't pass `:before_refresh` to Client constructors in `CognitoIdentityCredentials` (#2690).

1.40.0 (2022-02-24)
------------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,16 @@ def initialize(options = {})
@logins = options.delete(:logins) || {}
@async_refresh = false

client_opts = {}
options.each_pair { |k,v| client_opts[k] = v unless CLIENT_EXCLUDE_OPTIONS.include?(k) }

if !@identity_pool_id && !@identity_id
raise ArgumentError,
'Must provide either identity_pool_id or identity_id'
end

@client = options[:client] || CognitoIdentity::Client.new(
options.merge(credentials: false)
client_opts.merge(credentials: false)
)
super
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,19 @@ module CognitoIdentity
expect(creds.client).to be(client)
end

it 'excludes before_refresh from client construction' do
expect(CognitoIdentity::Client).to receive(:new)
.with({region: 'us-east-1', credentials: false})
.and_return(client)

creds = CognitoIdentityCredentials.new(
identity_id: identity_id,
region: 'us-east-1',
before_refresh: proc { }
)
expect(creds.client).to be(client)
end

it 'raises an argument error when identity_pool_id and identity_id are missing' do
expect { CognitoIdentityCredentials.new }.to raise_error(ArgumentError)
end
Expand Down
2 changes: 2 additions & 0 deletions gems/aws-sdk-core/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Unreleased Changes
------------------

* Issue - Don't pass `:before_refresh` to Client constructors in RefreshingCredential implementations (#2690).

3.130.1 (2022-04-12)
------------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def initialize(options = {})
options.each_pair do |key, value|
if self.class.assume_role_options.include?(key)
@assume_role_params[key] = value
else
elsif !CLIENT_EXCLUDE_OPTIONS.include?(key)
client_opts[key] = value
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def initialize(options = {})
options.each_pair do |key, value|
if self.class.assume_role_web_identity_options.include?(key)
@assume_role_web_identity_params[key] = value
else
elsif !CLIENT_EXCLUDE_OPTIONS.include?(key)
client_opts[key] = value
end
end
Expand Down Expand Up @@ -100,11 +100,10 @@ class << self
# @api private
def assume_role_web_identity_options
@arwio ||= begin
input = STS::Client.api.operation(:assume_role_with_web_identity).input
input = Aws::STS::Client.api.operation(:assume_role_with_web_identity).input
Set.new(input.shape.member_names)
end
end

end
end
end
2 changes: 2 additions & 0 deletions gems/aws-sdk-core/lib/aws-sdk-core/refreshing_credentials.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ module RefreshingCredentials
SYNC_EXPIRATION_LENGTH = 300 # 5 minutes
ASYNC_EXPIRATION_LENGTH = 600 # 10 minutes

CLIENT_EXCLUDE_OPTIONS = Set.new([:before_refresh]).freeze

def initialize(options = {})
@mutex = Mutex.new
@before_refresh = options.delete(:before_refresh) if Hash === options
Expand Down
10 changes: 7 additions & 3 deletions gems/aws-sdk-core/lib/aws-sdk-core/sso_credentials.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,13 @@ def initialize(options = {})
# validate we can read the token file
read_cached_token

options[:region] = @sso_region
options[:credentials] = nil
@client = options[:client] || Aws::SSO::Client.new(options)

client_opts = {}
options.each_pair { |k,v| client_opts[k] = v unless CLIENT_EXCLUDE_OPTIONS.include?(k) }
client_opts[:region] = @sso_region
client_opts[:credentials] = nil

@client = options[:client] || Aws::SSO::Client.new(client_opts)
@async_refresh = true
super
end
Expand Down
10 changes: 10 additions & 0 deletions gems/aws-sdk-core/spec/aws/assume_role_credentials_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ module Aws
expect(creds.client).to be(client)
end

it 'excludes before_refresh from client construction' do
allow(STS::Client).to receive(:new).with({credentials: false}).and_return(client)
creds = AssumeRoleCredentials.new(
role_arn: 'arn',
role_session_name: 'session',
before_refresh: proc {}
)
expect(creds.client).to be(client)
end

it 'accepts a client' do
creds = AssumeRoleCredentials.new(
client: client,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ module Aws
allow(client).to receive(:assume_role_with_web_identity).and_return(resp)
end

it 'contructs a default client when not given' do
it 'constructs a default client when not given' do
creds = AssumeRoleWebIdentityCredentials.new(
role_arn: 'arn',
web_identity_token_file: token_file_path,
Expand All @@ -59,6 +59,18 @@ module Aws
expect(creds.client).to be(client)
end

it 'excludes before_refresh from client construction' do
expect(STS::Client).to receive(:new).with({credentials: false}).and_return(client)

creds = AssumeRoleWebIdentityCredentials.new(
role_arn: 'arn',
web_identity_token_file: token_file_path,
role_session_name: "session-name",
before_refresh: proc { }
)
expect(creds.client).to be(client)
end

it 'auto populates :session_name when not provided' do
expect(client).to receive(:assume_role_with_web_identity).with({
role_arn: 'arn',
Expand Down
11 changes: 11 additions & 0 deletions gems/aws-sdk-core/spec/aws/sso_credentials_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,17 @@ def mock_token_file(start_url, cached_token)
expect(creds.client).to be(client)
end

it 'excludes before_refresh from client construction' do
expect(SSO::Client).to receive(:new)
.with({region: sso_region, credentials: nil})
.and_return(client)

mock_token_file(sso_start_url, cached_token)

creds = SSOCredentials.new(sso_opts.merge(before_refresh: proc {}))
expect(creds.client).to be(client)
end

it 'raises an argument error when arguments are missing' do
expect { SSOCredentials.new }.to raise_error(
ArgumentError, /Missing required keys/
Expand Down

0 comments on commit f5c29fc

Please sign in to comment.