Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stubbing issue with Presigner#presigned_url #2092

Closed
abMatGit opened this issue Jul 24, 2019 · 10 comments
Closed

Stubbing issue with Presigner#presigned_url #2092

abMatGit opened this issue Jul 24, 2019 · 10 comments
Labels
feature-request A feature should be added or improved. guidance Question that needs advice or information.

Comments

@abMatGit
Copy link

abMatGit commented Jul 24, 2019

Issue description

We are trying to eliminate AWS dependencies in our test environment. To do this we are stubbing the AWS client like so:

let(:aws_client_stub) { Aws::S3::Client.new(stub_responses: true) }
let(:presigner) { Aws::S3::Presigner.new(client: aws_client_stub) }

I can verify that this properly injects AWS stubbing handlers into the request execution:

[1] pry(#<Aws::S3::Presigner>)> req.handlers.entries.map(&:handler_class)
=> [Seahorse::Client::Plugins::Endpoint::Handler,
 Aws::Plugins::StubResponses::Handler,
...,
 #<Class:0x00007fe59c8cb6d8>]

The problem occurs when we try to presign the request, it hits this block of code which overwrites the stubbing handler (the #sign_but_dont_send method).

I'm not sure how exactly the handler stack is meant to be managed, but it is actually a hash where the :send key that was previously stored by Aws::Plugins::StubResponses::Handler is now handled by this block.

This is the handler stack after add_entry is run through:

[Seahorse::Client::Plugins::Endpoint::Handler,
 #<Class:0x00007fe593d36488>,
 ...
 #<Class:0x00007fe59c8cb6d8>]

Is there a way to fix this or persist the stubbing handler?

The ultimate goal we are trying to achieve is to stub the presigning request/response and achieve idempotency for testing. At the moment everytime we call Presigner#presigned_url we are getting a different result.

Gem name ('aws-sdk', 'aws-sdk-resources' or service gems like 'aws-sdk-s3') and its version

aws-sdk-s3 and aws-sdk-core

Version of Ruby, OS environment

Ruby-2.5.5

Code snippets / steps to reproduce

let(:client) { Aws::S3::Client.new(stub_responses: true) }
let(:presigner) { Aws::S3::Presigner.new(client: aws_client_stub) }
presigner.presigned_url(:get_object, bucket: <some_bucket>, key: 'global/default_images/ic_avatar_male.png') # <---- this should be idempotent because of stubbing
@cjyclaire
Copy link
Contributor

Looking at:

Aws> s = Aws::S3::Client.new(stub_responses: true)
=> #<Aws::S3::Client>
Aws> p = Aws::S3::Presigner.new(client: s)
=> #<Aws::S3::Presigner:0x007fdbcb4d9758 @client=#<Aws::S3::Client>>
Aws> p.presigned_url(:put_object, bucket: 'foo', key: 'bar')
[Aws::S3::Client 0 0.00196 0 retries] put_object(bucket:"foo",key:"bar")
=> "https://foo.s3.us-stubbed-1.amazonaws.com/bar?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=stubbed-akid%2F20190724%2Fus-stubbed-1%2Fs3%2Faws4_request&X-Amz-Date=20190724T222538Z&X-Amz-Expires=900&X-Amz-SignedHeaders=host&X-Amz-Signature=cdf2f20f10c2c6a4db73fc2964b2a3b69a138c4413198fa8e6e5d2c2ca4c6e7d"

It's using the stub client, the presigner its self is not making internet requests. The reason why the result is different everytime is because the signature v4 requires a timestamp, and by default it computing with Time.now using :expires_in provided.

You can stub Time.now in the test suit if you would like to have same url result every time, example can be found at our presigner spec

@cjyclaire cjyclaire added guidance Question that needs advice or information. closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jul 24, 2019
@abMatGit
Copy link
Author

abMatGit commented Jul 24, 2019

@cjyclaire Ah yes thank you for pointing that out!
Okay so actually using a stub client is almost entirely moot for the purpose of presigning (which makes sense -- should have caught that earlier).

Maybe something like this? (to avoid blanket time stubbing?):

timestamp = <some_static_timestamp>
allow(stubbed_presigner).to receive(:presigned_url).and_wrap_original do |m, *args|
  Timecop.freeze(timestamp) do
    m.call(*args)
  end
end

In either case, thank you for answering. I was missing some high level understanding of these classes so appreciate the explanation 🙏

@cjyclaire
Copy link
Contributor

No problem at all! looks good to me :D
Let me know you if you have further thoughts on easier stubbing even for those utility helper features, happy to tag those as feature requests ;)

@cjyclaire cjyclaire reopened this Jul 24, 2019
@cjyclaire cjyclaire added feature-request A feature should be added or improved. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jul 24, 2019
@jcohenho
Copy link

jcohenho commented Jan 29, 2022

@cjyclaire I'm using the following as in the linked spec file to stub the presigned_url timestamped signature.

before { allow(Time).to receive(:now).and_return(Time.utc(2022, 1, 28)) }

My test correctly runs and stubs the X-Amz-Signature param when run individually, however when I run my entire rspec test suite, it fails every time with a newly generated Signature. Is there some global pollution happening that could affect the Timestamped signature generation?

@alextwoods
Copy link
Contributor

There shouldn't be any global pollution happening from the SDK signers - but possible something in rspec/stubbing Time.now? Can you provide a larger example to repro?

@jcohenho
Copy link

jcohenho commented Jan 31, 2022

Here are two classes with passing unit tests when run by themselves (or one right after the other). s3_presigner_spec will always pass when run against the entire suite, while bin_zip_file_spec will only pass when run individually:

class S3Presigner

  def initialize(
    key:,
    bucket: ENV['S3_BUCKET'],
    expires_in: 1.hour.to_i,
    s3_client: Aws::S3::Client.new(region: ENV['AWS_REGION'])
  )
    @s3_client = s3_client
    @key = key
    @bucket = bucket
    @expires_in = expires_in
    @presigner = Aws::S3::Presigner.new(region: ENV['AWS_REGION'], client: @s3_client)
  end

  def presigned_url
    @presigner.presigned_url(:get_object, bucket: @bucket, key: @key, expires_in: @expires_in,
                              response_content_disposition: 'attachment')
  end
end
require 'spec_helper'

describe S3Presigner do
  let(:key) {'example/example-key'}
  let(:bucket) {'example-bucket'}

  # Stub time to create a non-unique timestamp for filepaths in the URL: https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-s3/spec/presigner_spec.rb#L21
  before { allow(Time).to receive(:now).and_return(Time.utc(2022, 1, 28)) }

  describe '#presigned_url' do
    it 'generates a presigned url' do
      expected_url = 'https://example-bucket.s3.us-stubbed-1.amazonaws.com/example/example-key?'\
                     'response-content-disposition=attachment'\
                     '&X-Amz-Algorithm=AWS4-HMAC-SHA256'\
                     '&X-Amz-Credential=stubbed-akid%2F20220128%2Fus-stubbed-1%2Fs3%2Faws4_request'\
                     '&X-Amz-Date=20220128T000000Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host'\
                     '&X-Amz-Signature=f1e47c93c5f94f58372923eea07179943e2ac970ac7d49e4ce5f3c7bf2'

      s3_client = Aws::S3::Client.new(stub_responses: true)
      presigner = described_class.new(s3_client: s3_client, key: key, bucket: bucket)
      actual_url = presigner.presigned_url
      expect(actual_url).to eq(expected_url)
    end
  end
end

Not sure if it's worth sharing the code for this second test, but I'm happy to if needed. This test will pass when run by itself, but then fail when I run the entire test suite, with a difference in the signature:

require 'spec_helper'

describe BinZipFile do
  subject { described_class }

  let(:user) {create(:user)}
  let(:video_1) {create(:video)}
  let(:video_2) {create(:video)}
  let(:bin) {create(:bin, title: 'test bin', user: user, videos: [video_1, video_2])}
  let(:aws_client_stub) { Aws::S3::Client.new(stub_responses: true) }
  let(:bucket) {'example-bucket'}
  let(:bin_zip_file) {described_class.new(bin: bin, user: user, s3_client: aws_client_stub, bucket: bucket)}

  # Stub time to create a non-unique timestamp for filepaths in the URL: https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-s3/spec/presigner_spec.rb#L21
  before {allow(Time).to receive(:now).and_return(Time.utc(2022, 1, 28))}

  it 'creates a presigned url for a zip file on s3' do
    expected_presigned_url = "https://example-bucket.s3.us-stubbed-1.amazonaws.com/bins/#{bin.id}/test_bin_20220128T000000.zip"\
                             "?response-content-disposition=attachment"\
                             "&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=stubbed-akid%2F20220128%2Fus-stubbed-1%2Fs3%2Faws4_request"\
                             "&X-Amz-Date=20220128T000000Z&X-Amz-Expires=3600"\
                             "&X-Amz-SignedHeaders=host"\
                             "&X-Amz-Signature=b45652409d3300ac6edf910ab40295e07ee26166ac59d18bdf2f9c4283"
    expect(bin_zip_file.create).to eq expected_presigned_url
  end
end

Here is the failure for when it's run with the entire test suite, you can plainly see the difference in signatures. The signature will change every time the entire suite is run. You can also see that the filename is correctly stubbed as it includes the timestamp in the name:

     Failure/Error: expect(bin_zip_file.create).to eq expected_presigned_url

expected:     
https://example-bucket.s3.us-stubbed-1.amazonaws.com/bins/5/test_bin_20220128T000000.zip?response-content-disposition=attachment&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=stubbed-akid%2F20220128%2Fus-stubbed-1%2Fs3%2Faws4_request&X-Amz-Date=20220128T000000Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Signature=b652409d33b46ac6edf910ab40295e07ee26166ac59d1df2f9c4283

actual:
https://example-bucket.s3.us-stubbed-1.amazonaws.com/bins/5/test_bin_20220128T000000.zip?response-content-disposition=attachment&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=stubbed-akid%2F20220128%2Fus-stubbed-1%2Fs3%2Faws4_request&X-Amz-Date=20220128T000000Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Signature=2f179f3e3752f64e4be25780886f5747d783b1f88eb37789ca78f5d38



@alextwoods
Copy link
Contributor

Any chance any of the other content is changing between tests? Does the x-amz-date header change? I can't see from the diff you have above - but thats the header used in creating the signature.

To debug further, you could try stubbing the Signer#sign_request method to print out the requests and compare them.

@jcohenho
Copy link

@alextwoods sorry, I updated my previous comment to include the full URLs. x-amz-date is the same in both results, only the signature is different. Can you link me to the sign_request method? I can't seem to locate which one you are referring to. Thank you!

@alextwoods
Copy link
Contributor

Edit: Sorry, should have been the presign_url method - https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sigv4/lib/aws-sigv4/signer.rb#L377

@jcohenho
Copy link

@alextwoods you were indeed correct in your assumptions- after debugging, I realized that the URL changes based on the ID of the bin factory, but I wasn't taking the filename change into consideration when generating the signature. Everything is working now across tests, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

4 participants