zooniverse / panoptes

Zooniverse API to support user defined volunteer research projects
Apache License 2.0
103 stars 41 forks source link

Empty response body when failing to authenticate with client credentials #2257

Open amy-langley opened 7 years ago

amy-langley commented 7 years ago

We had a problem where caesar was configured with invalid credentials. It was throwing a rather cryptic error: JSON::ParserError: A JSON text must at least contain two octets!

It turns out that this was because requesting oauth/token with invalid credentials is returning an empty response body with an HTTP 422 error, but @zwolf said that it should return a JSON object that includes an error message.

I'm going to try to modify the faraday panoptes middleware to behave more sensibly (in this case, fail more legibly), but I'm entering this issue in case there is a problem with the oauth stuff we're doing that is resulting in this behavior. I've attached a VCR fixture that shows the failing request.

amy-langley commented 7 years ago

This is the failed request:

should_make_a_get_request.yml.txt

---
http_interactions:
- request:
    method: post
    uri: https://panoptes-staging.zooniverse.org/oauth/token
    body:
      encoding: UTF-8
      string: client_id=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx&client_secret=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx&grant_type=client_credentials
    headers:
      User-Agent:
      - Faraday v0.11.0
      Content-Type:
      - application/x-www-form-urlencoded
      Accept-Encoding:
      - gzip;q=1.0,deflate;q=0.6,identity;q=0.3
      Accept:
      - "*/*"
  response:
    status:
      code: 422
      message: Unprocessable Entity
    headers:
      Cache-Control:
      - no-cache
      Content-Type:
      - text/html
      Date:
      - Tue, 21 Mar 2017 19:36:37 GMT
      P3p:
      - CP="NOI ADM DEV PSAi COM NAV OUR OTRo STP IND DEM"
      Server:
      - nginx/1.10.0 (Ubuntu)
      Strict-Transport-Security:
      - max-age=31536000; includeSubDomains
      Vary:
      - Origin
      X-Content-Type-Options:
      - nosniff
      X-Frame-Options:
      - SAMEORIGIN
      X-Request-Id:
      - a74caeac-7a1a-41e5-8463-0282bff25444
      X-Runtime:
      - '0.004838'
      X-Xss-Protection:
      - 1; mode=block
      Content-Length:
      - '0'
      Connection:
      - keep-alive
    body:
      encoding: UTF-8
      string: ''
    http_version: 
  recorded_at: Tue, 21 Mar 2017 19:36:37 GMT
recorded_with: VCR 3.0.3
amy-langley commented 7 years ago

Related to zooniverse/faraday-panoptes#1

zwolf commented 7 years ago

I said that it should return a JSON object that includes an error message. Even if we technically should be returning a error object with the 422, the status code is probably enough to go on.

amy-langley commented 7 years ago

Sorry, yeah, that's more sensible. As you can see in the referenced PR I'm now checking for an empty response body or a 422 and throwing an actually meaningful exception so that it can either be a) caught and handled by client code or b) will at least show up in the logs as something we can understand.

amy-langley commented 7 years ago

I am wondering if someone thought ''.to_json seemed redundant and replaced it with '' because that is an easy mistake to make and would also produce the error we're seeing

camallen commented 7 years ago

I thinks this is related to the custom security of the doorkeeper app here the head response would explain the busted body json parsing.

zwolf commented 7 years ago

Yeah, I wasn't clear on whether that was applying the header/code or if it was rendering it, but it's def tossing those back with nothing included. Rendering a spec-compliant message instead's a good idea.

amy-langley commented 7 years ago

I did put a workaround in our client middleware if it turns out this is too much of a hassle to deal with. It's mostly a diagnostic issue the next time something is misconfigured :)

camallen commented 7 years ago

Decent response body messaging on this would be super useful as y'all found out ;)