Use authorization header instead of sending token via param

[chrisbloom7]

A change to the Slack API earlier this year prevents tokens from being specified as querystring URLs. Even though the Slack API documentation for POST endpoints says that we should still be able to send tokens in the body of POST requests, this does not seem to be the case in practice*:

$ export SLACK_API_TOKEN="xapp-1-XXXXXXX"
$ curl --request "POST" \
  --header "Content-Type:application/x-www-form-urlencoded" \
  --header "Authorization: Bearer $SLACK_API_TOKEN" \
  https://slack.com/api/apps.connections.open
{"ok":true,"url":"wss:..."}

$ curl --request "POST" \
  --header "Content-Type:application/x-www-form-urlencoded" \
  --data "token=$SLACK_API_TOKEN" \
  https://slack.com/api/apps.connections.open
{"ok":false,"error":"invalid_auth"}

This prevents certain web client methods from being used, such as apps_connections_open. This PR updates the web client request module to send the client token as an authorization header rather than as a request parameter, which allows these methods to be used again.

Fixes https://github.com/slack-ruby/slack-ruby-client/issues/363

Reviewer note: I was unable to complete the Update Slack Web API step in the Contributions guide. On my first attempt, the rake task failed with Cannot rebase onto multiple branches. I manually ran the commands in slack:git_update rake task and tried again, this time resulting in a failed to apply patch error. I reset all the changes and tried following the steps under Resolving Patch Errors, but that resulted in another error rake aborted!\nMissing group chat.

* I confirmed with Slack support that the documentation is indeed incorrect, and you can not pass the token in a POST body parameter. They are working on updating their API documentation.

[chrisbloom7]

@dblock I saw your note here about needing to change a bunch of the VCR cassettes. One obstacle to that is that the tests don't currently pass on master if you enforce anything beyond matching cassettes on method and URI (the defaults).

VCR.configure do |config|
  config.default_cassette_options = {
    match_requests_on: %i[method uri body],
  }
  ...
end

$ bundle exec rake
475 examples, 54 failures, 8 pending

That poses two problems:

1. Updating the cassette fixtures becomes meaningless because the request body and headers aren't checked in tests
2. It's difficult to see which fixtures would need to change for this PR

Do you have any suggestions on how to address this in this PR?

PS: If one adds headers to the match_requests_on option then even more tests fail because the User-Agent headers were captured by different versions of the gem, so they'd have to be blanket updated on every version bump, or you'd need a custom header matcher that ignored the User-Agent header, or a hook that stripped that header before recording.

[dblock]

What’s the issue on master?

With a change like this I would delete all existing VCRs and re-record them, modifying any tests that need to be modified.

[chrisbloom7]

Sorry if that comment wasn't clear. Right now, the tests match VCR cassettes on the request method and the URI, which is the default config for VCR. If a test request sends different headers or request body content, the tests won't complain and everything looks green. If you change the VCR config to also match headers and/or body content then the tests on master start failing because the cassettes have fallen out of sync with the requests that the app is currently making - headers have changed as has body content. This is problematic for this PR for the reasons outlined above: Making changes to request headers and body content in existing VCR cassettes doesn't matter because the tests ignore them, and it's difficult to know which cassettes would need to be updated because so many are failing even before these changes if we start matching on headers and body content.

That said, if your recommendation is to just delete them all and re-record them then it's a mostly moot argument, though I would strongly suggest that we still update the VCR config which would make it easier to just update certain cassettes as necessary and validate the headers and body content are what we expect. In this case, do you have a Slack team that you use specifically for this, or do I need to setup a new one and configure a bot? Do you have documentation for this process anywhere?

[dblock]

Sorry if that comment wasn't clear. Right now, the tests match VCR cassettes on the request method and the URI, which is the default config for VCR. If a test request sends different headers or request body content, the tests won't complain and everything looks green. If you change the VCR config to also match headers and/or body content then the tests on master start failing because the cassettes have fallen out of sync with the requests that the app is currently making - headers have changed as has body content. This is problematic for this PR for the reasons outlined above: Making changes to request headers and body content in existing VCR cassettes doesn't matter because the tests ignore them, and it's difficult to know which cassettes would need to be updated because so many are failing even before these changes if we start matching on headers and body content.

Understood.

That said, if your recommendation is to just delete them all and re-record them then it's a mostly moot argument, though I would strongly suggest that we still update the VCR config which would make it easier to just update certain cassettes as necessary and validate the headers and body content are what we expect.

Yes, this is a good point and let's definitely.

In this case, do you have a Slack team that you use specifically for this, or do I need to setup a new one and configure a bot? Do you have documentation for this process anywhere?

I don't. I use my own, incrementally, and make sure we never write any real tokens in the VCR K7s. When re-recording you'll probably have to update a million things, but I don't know a better way.

[chrisbloom7]

OK, thanks for that detail. I'll take a crack at it asap and let you know if I run into any problems that feel outside the scope of this PR

[chrisbloom7]

I took a crack at this this today over several hours but have been unsuccessful.

Recording cassettes from scratch is going to be a very big undertaking given that the tests are written against responses from whatever team they were pointed at when they were initially created, and they expect certain channel IDs and names to be present, or specific pages of results. The IDs cannot be duplicated since they are globally unique, and there's no documentation for what channel names or content needs to exist in whatever team I point the tests at.

I also tried just getting the master branch to pass while comparing body contents as a first step and even that is proving to be very frustrating as some tests were recorded with an empty API token but don't explicitly unset the token in their setup, and updating them to do so seems to lead to cascading test failures. Additionally, some cassettes responses seem to have been reused for API calls they weren't recorded with, i.e. multiple tests for the same endpoint reuse cassettes even though they each actually generate different body params.

I'm unsure where to go from here. I don't have the resources to work through this test by test to recreate the necessary Slack team to test against, while leaving the cassettes as-is and ignoring the changes to the request body and headers seems like it will cause much confusion for future contributors.

[dblock]

@chrisbloom7 Let me help? Put up a PR with just K7s re-run and I'll fix the tests or maybe I still have some of the setup that hasn't changed too much - put up code without the K7 changes and I'll try to re-record them?

[chrisbloom7]

If you can get tests passing on master with this VCR config, with or without starting from scratch, that would be a huge help.

VCR.configure do |config|
  # ...
  config.default_cassette_options = {
    match_requests_on: %i[method uri body],
    record: :once,
  }
end

If we can get to green with that, then I can just update the cassettes with the changes to the body and headers and feel confident that the changes are covered

[dblock]

If you can get tests passing on master with this VCR config, with or without starting from scratch, that would be a huge help.

VCR.configure do |config|
  # ...
  config.default_cassette_options = {
    match_requests_on: %i[method uri body],
    record: :once,
  }
end

If we can get to green with that, then I can just update the cassettes with the changes to the body and headers and feel confident that the changes are covered

Does this work: https://github.com/slack-ruby/slack-ruby-client/pull/390 ?

[chrisbloom7]

@dblock yes, looks great!

[chrisbloom7]

I can rebase on top of that once it's merged, and then add the header checks and update the cassettes

[dblock]

I can rebase on top of that once it's merged, and then add the header checks and update the cassettes

Merged

[dblock]

Would it be possible to add a VCR spec, maybe for auth, that has an Authorization: token header and that would break if it's accidentally removed?

[dblock]

Btw @chrisbloom7, care to take a look at https://github.com/slack-ruby/slack-api-ref/issues/47? I think we are missing tons of updates from slack api.

[chrisbloom7]

Hmm, interestingly, with https://github.com/slack-ruby/slack-ruby-client/pull/390/files#diff-3b4851931d0c5827506a8e33e4d96d61193a850014364a50c92f75ff6467a977R32 in place from your test update most of the old token assignments have been stripped from the cassettes, meaning few will need to be changed. I'll update the auth specs specifically so we are covered, as you suggested.

[chrisbloom7]

@dblock when you updated the cassettes, I assume you must have had a token set to re-run all the interactions, but all of the token=token have been stripped from the request body in the cassettes except for a few. Any idea how that happened? I was counting on the token=token in the cassette request body to cause tests to fail again now that we've moved that to a header. It's not a huge deal and I can work around it, but it's surprising.

[dblock]

@dblock when you updated the cassettes, I assume you must have had a token set to re-run all the interactions, but all of the token=token have been stripped from the request body in the cassettes except for a few. Any idea how that happened? I was counting on the token=token in the cassette request body to cause tests to fail again now that we've moved that to a header. It's not a huge deal and I can work around it, but it's surprising.

I think I got lazy and this was how I solved matching the body when switching between real/fake token. Let me take a look.

[dblock]

I merged https://github.com/slack-ruby/slack-ruby-client/pull/391, does this work?

[chrisbloom7]

@dblock perfect! I've updated the tests for the new auth header.

[dblock]

Squashed and merged, thank you! I'd like to get the api updated (https://github.com/slack-ruby/slack-api-ref/issues/47) and make a new release soon. Any interest in helping co-maintain the gem @chrisbloom7? Drop me an email to dblock at dblock dot org with your rubygems login if you want in.