783598 - Make conditions required response easier to detect

Status: VERIFIED WONTFIX

(Cloud Services Graveyard :: Server: Token, defect)

Description

[Gregory Szorc [:gps]]

The current "conditions required" HTTP response looks like:

  200 OK
  Content-Type: application/json

  {"status": 403,
   "errors": [{"description": "Need to accept conditions",
               "location": "body",
               "name": ""}],
   "urls": {"tos": "http://example.com"}
  }


The heuristic for clients to detect this involves 3 steps:

1) Status == 403
2) Content-Type is application/json
3) JSON body contains "urls" field

One issue is the generic "urls". At the least I think this should be "conditions_urls" or similar. The reason is I can see some other error handler recycling "urls" for its own use (it is generic, after all). That would throw off clients.

Secondly, I don't like that the token server response message type is being determined by the presence of a field specific to that type. In other words, we are looking for traits to identify response message types instead of looking at a messageType field, different media type, other response header, etc. If we did this, my concern about "urls" would go away.

While I have these concerns for all HTTP APIs, I am more concerned about it for 4xx and 5xx status codes. From my experience, misconfigured or misbehaving servers often will generate their own 4xx and 5xx response messages. In the absence of a clear and identifying property of the response, it is all too easy for clients to confuse actual token server responses with those generated by intermediate HTTP agents. This will cause client bugs.

So, what I guess I'm asking for is something in the HTTP response that says without a doubt that the "conditions required" payload is actually that.

Comment 1

[Ryan Kelly [:rfkelly]]

I propose something like the following:

   403 Forbidden
   Content-Type: application/json
 
   {"status": 403,
    "errors": [{"description": "Need to accept conditions",
                "location": "header",
                "name": "X-Conditions-Accepted",
                "error_code": 42,
                "condition_urls": {"tos": "http//example.com},
              }],
   }


Key changes:

  * The location of the error is a specific header, not "body", so set the fields on
    the error object to reflect that.

  * The "urls" field belongs to one specific error, so it should live inside the specific
    error object rather than at the top level.  Also renamed, although I think that's less
    of an issue after moving it.

  * Added an "error_code" field to the error object.  This is not strictly necessary in
    this case because the error already identifies the specific header at fault.  But for
    other cases it may be useful for machines to distinguish between e.g. "the header was
    missing" and "the header was malformed".


Thoughts?

Comment 2

[Gregory Szorc [:gps]]

+1

Comment 3

[Alexis Metaireau (:alexis)]

+1 for this as well.

The error_code thing should go in a different issue IMO.

Comment 4

[Alexis Metaireau (:alexis)]

This should be handled by https://github.com/mozilla-services/cornice/pull/65 and https://github.com/mozilla-services/tokenserver/pull/13

Ryan can you review this?

Comment 5

[Ryan Kelly [:rfkelly]]

r+

Comment 6

[Ryan Kelly [:rfkelly]]

I have filed Bug 786523 to track the addition of an error_code field.

Comment 7

[Ryan Kelly [:rfkelly]]

These were committed in relevant repos, closing out this bug.

Comment 8

[Ryan Kelly [:rfkelly]]

I am tentatively re-opening this in light of Bug 792674.

Comment 9

[Ryan Kelly [:rfkelly]]

Linking to sync+fxa bugtree for consideration, although I suspect we'll triage it back out in short order.

Comment 10

[Ryan Kelly [:rfkelly]]

conditions-accepted was removed from tokenserver 1.0 spec, unblocking the metabug

Comment 11

[Ryan Kelly [:rfkelly]]

conditions-required is no longer required, I think we can just delete this.

Comment 12

[James Bonacci [:jbonacci]]

OK. Good.