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

Authentication tokens/step in pact. #49

Closed
stefan-lz opened this issue Oct 5, 2014 · 26 comments
Closed

Authentication tokens/step in pact. #49

stefan-lz opened this issue Oct 5, 2014 · 26 comments

Comments

@stefan-lz
Copy link

So at the moment we are having to modify the pact.json files before running pact. This is for inserting authentication tokens with expiry in about 2 hours into the pacts.

pacts = File.join(File.dirname(File.expand_path(__FILE__)), '../src/test/resources/pacts/*.json')
Dir.glob(pacts).each do |f|
  text = File.read(f)
  output_of_gsub = text.gsub(/\"Authorization\"\s*:\s*\".+\"/) { "\"Authorization\": \"ioof-token #{token}\"" }
  File.open(f, "w") { |file| file.puts output_of_gsub }
end

It would be good to be able to nicely insert/modify headers of the request before they are sent without modifying the pact files.

Cheers,

@uglyog
Copy link
Member

uglyog commented Oct 5, 2014

For the Gradle Pact-JVM provider plugin, I added a callback that allows the request to be modified. See https://github.com/DiUS/pact-jvm/tree/master/pact-jvm-provider-gradle#modifying-the-requests-before-they-are-sent

@bethesque
Copy link
Member

I feel very uncomfortable with this idea. If you can modify the request, then there is no guarantee that the actual request that the service will get will look like what gets replayed in pact:verify. Are any of the following options for you?

  1. Setting the time using Timecop for both Consumer and Provider tests.
  2. Making the expiry period configurable, and setting it to a very large time in the set_up of the provider state.

@bethesque
Copy link
Member

Ron, in that scenario, the Consumer could completely miss sending the OAUTH token, and nothing in the tests would break. I understand making the request available as readonly, so that you could inspect the token, and then set up the appropriate state on the provider so that the token would work, but I feel very strongly against allowing the request to be modified.

@stefan-lz
Copy link
Author

@bethesque, I agree that modifying the pacts make them less trustworthy.

So I guess the issue is in our current testing environment we do not have any mock or stub service when authenticating. So if we're using a real authenticator service, we need to have a real auth-token in the request.

@bethesque
Copy link
Member

Are you executing pact:verify against a rack app, or are you using pact-provider-proxy to run pact:verify against an actual running server?

@uglyog
Copy link
Member

uglyog commented Oct 6, 2014

@bethesque In the JVM version, the provider is running as a separate server so there is no way to setup anything in the provider. The only way to do it would use a mock server for the authentication. In our particular case we don't have control over the authentication, we don't have a mock server and the tokens are time sensitive and required to be signed with RSA keys.

I believe as framework authors we should allow people to be able to use the framework in any manner they require that is helpful to them, and not be prescriptive. So even though they may be able to shoot themselves in the foot, I say 'Happy Shooting'.

@bethesque
Copy link
Member

Sigh. It makes me sad, but I see your point Ron.

@stefan-lz How are you verifying? With the normal Ruby pact:verify? Or against a running endpoint?

@stefan-lz
Copy link
Author

pact:verify running on j-ruby (not sure if that matters or not) and our
provider is a jetty drop-wizard app, so we don't have any nice VCR/Timecop
options either. Cheers.

On Monday, October 6, 2014, Beth notifications@github.com wrote:

Sigh. It makes me sad, but I see your point Ron.

@stefan-lz https://github.com/stefan-lz How are you verifying? With the
normal Ruby pact:verify? Or against a running endpoint?


Reply to this email directly or view it on GitHub
https://github.com/realestate-com-au/pact/issues/49#issuecomment-57977345
.

Stefan Leszkiewicz

DiUS Computing Pty. Ltd.
Where Ideas Are Engineered

Phone: +61 3 9008 5400
Mobile: +61 402 62 9802

www.dius.com.au

@bethesque
Copy link
Member

So, I've had a think about the nicest way we can allow people to shoot themselves in the foot.

Pact.provider_states_for "My Jetty DropWizard App whatever that is" do

  set_up do | interaction |
    interaction.request.headers['Authorization'] = 'some dynamic token'
  end

end
$ rake pact:verify
Note: set_up hook modified request:
{
  "headers" : {
-   "Authorization" : "original token"
+   "Authorization" : "some dynamic token" 
  }
}

The set_up hook already exists, it just needs to be modified to yield a copy of the interaction that can then be compared to the original one, and an information message logged to the screen to alert the Shooter to the fact that they are shooting themselves.

@bethesque
Copy link
Member

So, @stefan-lz, if you want to make the above happen, I will reluctantly accept a pull request.

@bethesque
Copy link
Member

Btw, I really think that instead, you should create a mock authentication service. This feels dirty.

@NigelThorne
Copy link

I think if something feels dirty you shouldn't do it... Leave it in a
branch.

If you wait long gone enough another option will become obvious that
satisfies this need and several others as well.
On 6 Oct 2014 21:13, "Beth" notifications@github.com wrote:

Btw, I really think that instead, you should create a mock
authentication service. This feels dirty.


Reply to this email directly or view it on GitHub
https://github.com/realestate-com-au/pact/issues/49#issuecomment-57997745
.

@stefan-lz
Copy link
Author

I've written a spike getting dynamic auth tokens into the request via provider setup. The commit is here: stefan-lz@18c247a It does not include creating the diff output yet.

It's all very hacky, and probably breaks plenty of rules, so I'm not going to submit a pull request. But it works, and the tests are passing.

I particually like this line: stefan-lz@18c247a#diff-6ba38645571c7cc5e2d9ad8f717924e3R23

I'm now divided wether this is a good thing or not, and our case is quite unique. So I'm happy to close this until there is a larger pressing need for it.

@bethesque
Copy link
Member

I don't know quite how drop wizard works, but do you have a rack app interface, or are you using pact-provider-proxy?

@bethesque
Copy link
Member

If you are using the normal pact:verify, then you could something like this: https://gist.github.com/bethesque/203e9d2ec08f4a40d2df
No dirty hacks required!

I think there's a similar thing you could do if you are using pact-provider-proxy.

@stefan-lz
Copy link
Author

Ah nice 1, but we are not using rack unfortunately. Its running in its own separate process, outside of ruby. Might as well be a server half way across the world somewhere. A proxy server would probably work, but also a fair bit of effort.

@bethesque
Copy link
Member

I don't understand how you are running pact:verify if you are not using pact-provider-proxy and it's not a rack app?! Can you show me the pact:verify configuration??

@stefan-lz
Copy link
Author

It could be a proxy setup, maybe - but spawning an external process within. I'm not 100% on how it hangs together, we do need to fire up the server first before running pact.

Here's our whole setup:

  • jruby-1.7.9
  • pact (1.3.3)
  • pact-provider-proxy (1.1.0)

In our Rakefile we have this:

require 'pact/tasks'
require 'pact/provider/proxy/tasks'
require_relative 'lib/dropwizard/app'
...
pacts = Dir['./src/test/resources/pacts/*'].map do |file_name|
  pact_name = file_name[file_name.rindex('/')+1..file_name.rindex('.')-1]
  {:name => pact_name, :src => file_name}
end

pacts.each do |pact|
  Pact::ProxyVerificationTask.new pact[:name] do |task|
    task.pact_url pact[:src], :pact_helper => './lib/pact_helper'
    task.provider_base_url 'http://localhost:9999'
  end
end
...
namespace :pact do
  desc 'verify interactions with service'
  task :verify_all do
    pacts.each do |pact|
      Rake::Task["pact:verify:#{pact[:name]}"].execute
    end
  end
end

And we run our provider tests using rake pact:verify_all after firing up our server.

We also have a rake task to do this:

task :integration do
     ...
      #update pact files with tokens here
      ...
begin

      Rake::Task['server:start_server'].execute
      Rake::Task['spec'].execute
      Rake::Task['pact:verify_all'].execute
    ensure
      Rake::Task['server:stop_server'].execute
    end
end

Our lib/dropwizard/app.rb (proxy?) looks like this:

module Dropwizard
  class App

    def self.start_server
      begin
        puts 'waiting for server to start...'
        sleep 1
        started = HTTParty.get(URI::encode('http://localhost:9999/admin/healthcheck')).success?
      rescue
        @pid = Process.spawn("#{command} server ./conf/test/config.yml") unless @pid or started
      end until started
    end

    def self.stop_server
      if @pid
        puts "killing server on #{@pid}"
        system "kill #{@pid}"
      end
    end

    def self.command
      "java -jar ./target/service*.jar"
    end

  end
end

and here is a snippet of our pact_helper.rb:

Pact.set_up do
  DatabaseHelper.clean(true)
end

def create_standard_config
      create_group_limit("ABC", 2)
      ...
end

Pact.provider_states_for 'ABC_Web' do
  provider_state 'Groups for an advisor and groups for other advisors' do
    set_up do
      create_standard_config()
      create_group(1, 'group1')
      ...
    end
  end
...
end

Its a bit glued together, but its been working for us. The main problem with this setup is modifying the pact files which are checked into git, so this means we have to check them out again after every run.

@bethesque
Copy link
Member

So you are using pact-provider-proxy! See that 'require "pact/provider/proxy"' line :P

Ok, I have an idea of how to do this more elegantly, but putting in another rack app between the Rack::ReverseProxy and Pact, in here:

https://github.com/bethesque/pact-provider-proxy/blob/master/lib/pact/provider/proxy/proxy_pact_helper.rb

class AddAuthenticationHeaders 
   def initialize app
     @app = app
   end

  def call env
    @app.call(env.merge('HTTP_AUTHORIZATION_HEADER_THING' => 'new_header'))
  end
end

end


Pact.service_provider "Running Provider Application" do
  app do
    reverse_proxy = Rack::ReverseProxy.new do
      reverse_proxy '/', ENV.fetch('PACT_PROVIDER_BASE_URL')
    end
    AddAuthenticationHeaders.new(reverse_proxy)
  end
end

@eskimoquinn
Copy link

@bethesque I have found a few examples of how to modify the headers, like your example above, can the request body be similarly modified? If so, it is not apparent to me how to do that. Any suggestions?

@bethesque
Copy link
Member

Yes, using the same approach (though can I stress, it is NOT recommended!) What problem are you trying to solve? Before we go down the dirty hack route, let's see if there's another way we can solve it.

@bethesque
Copy link
Member

Btw, for anyone reading this issue in the future, there is a --custom-provider-header option you can use in the pact-provider-verifier now. See docs here: https://github.com/pact-foundation/pact-provider-verifier#setting-a-custom-authentication-header

@eskimoquinn
Copy link

eskimoquinn commented Sep 6, 2018

@bethesque the reason I asked this question about modifying body is because we are trying to test services that wrap an externally hosted database. We need to test deleting an entity, however, we can only delete by a uniquely generated entity ID. We cannot setup the entity with a specific ID. Thus we need to modify the ID of the entity in the request before making the call against the real service. I see that the JVM provider has a way to do this, but we would really like to do this in the pact-provider-proxy for Ruby. We still have this problem by the way and it is leading to people not writing tests for these endpoints or questioning pact in general. Any thoughts?

@bethesque
Copy link
Member

Yes. I've just written some rack middleware to allow the request to be modified in transit. There will be a more elegant implementation of this in the v4 spec, but for now, I would use a hardcoded token that you replace in this middleware.

The way you use it is to create a middleware class that implements Pact::ProviderVerifier::CustomMiddleware and modifies the Rack env. The Rack env is just a big flattened hash of all the request information. I've augmented it with the provider states information.

class MyMiddleware < Pact::ProviderVerifier::CustomMiddleware
  def initialize app
    @app = app
  end

  def call env 
    provider_states = provider_states_from(env)
    provider_state_name = provider_states.any? && provider_states.first.name
    puts "The provider state name is '#{provider_state_name}'"
    env['PATH_INFO'].gsub('HARDCODED_ID', 'DYNAMIC_ID') # Obviously, this has to be gotten from somewhere dynamically
    @app.call(env)
  end
end

If writing code in ruby is not your thing, you could create a proxy app in the language of your choice that modifies the request appropriately.

@eskimoquinn
Copy link

Thanks @bethesque we are trying this out.

@YOU54F
Copy link
Member

YOU54F commented Jun 25, 2024

Closing this as per the comments here

#49 (comment)

@YOU54F YOU54F closed this as completed Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants