Add conditions acceptance API to token server client

RESOLVED FIXED in mozilla18

Status

Cloud Services
Firefox: Common
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 652639 [details] [diff] [review]
Add conditions acceptance to client, v1

The token server now has a "conditions accepted" feature. It isn't documented at docs.services yet (at least not until bug 782790).

The attached patch adds support for the JS client.

Review goes to mconnor for code and rfkelly for behaviour.
Attachment #652639 - Flags: review?(mconnor)
Attachment #652639 - Flags: feedback?(rfkelly)
Comment on attachment 652639 [details] [diff] [review]
Add conditions acceptance to client, v1

One thing that's not quite right is the use of the "description" field in the JSON.  It is not a top level field, although I can see why you would have inferred that from what I said on IRC.  It is instead buried inside an "errors" list like this:

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

This is the generic error-reporting format used by cornice, allow to report multiple errors in a single response.  It may be simpler to check for the presence of the "errors" field rather than trying to dig down and find this description.

I'm going f- because of the above, but other than that this all looks good to me.  Nice documentation and API design to encourage the sending of the flag only when appropriate.
Attachment #652639 - Flags: feedback?(rfkelly) → feedback-
(Assignee)

Comment 2

6 years ago
Comment on attachment 652639 [details] [diff] [review]
Add conditions acceptance to client, v1

Gah. I guess this is a good excuse to go all out and implement parsing of Cornice's error responses.
Attachment #652639 - Flags: review?(mconnor)
(Assignee)

Comment 3

6 years ago
Created attachment 652884 [details] [diff] [review]
Add conditions acceptance to client, v2

I updated the response format.

I also changed how the error system works. Before, we lumped all server errors into a generic TokenServerClientServerError instance and clients had to do some extra work to figure out what to do. I've added this functionality to the token server client and exposed the results through properties on those error instances.

The special flags only get populated if we receive a Cornice error report from the server. This requires Content-Type to be application/json and for the payload to be the Cornice JSON blob. I /think/ this is a fair assumption, but I'd like confirmation from the Cornice people.
Assignee: nobody → gps
Attachment #652639 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #652884 - Flags: review?(mconnor)
Attachment #652884 - Flags: feedback?(tarek)
Attachment #652884 - Flags: feedback?(rfkelly)
Comment on attachment 652884 [details] [diff] [review]
Add conditions acceptance to client, v2

Your patch still has a comment saying "Currently most non-200 status codes are rolled into one error type", should this now be reverted to the previous statement that "Currently all non-200 status codes are rolled into one error type"?

Server components upstream of the actual python code could easily result in non-cornice-format error documents.  For example, I think the 404 document is generated by pyramid and is a generic HTML template, not cornice-format json.  Any 503 responses will come directly from zeus using its own format.

But assuming that those other types of response are already handled by code not in this patch, this looks OK to me.
Attachment #652884 - Flags: feedback?(rfkelly) → feedback+
(Assignee)

Comment 5

6 years ago
Comment on attachment 652884 [details] [diff] [review]
Add conditions acceptance to client, v2

rfkelly took care of it.
Attachment #652884 - Flags: feedback?(tarek)
Comment on attachment 652884 [details] [diff] [review]
Add conditions acceptance to client, v2

Review of attachment 652884 [details] [diff] [review]:
-----------------------------------------------------------------

Round again.

::: services/common/tests/unit/test_tokenserverclient.js
@@ +71,5 @@
>    run_next_test();
>  });
>  
> +add_test(function test_conditions_required_response_handling() {
> +  _("Ensure that a conditions required response is handled properly.");

You're not testing malformed urls. I don't think you'll ever see

  		this._log.warn("403 response without proper fields!");

get hit...

::: services/common/tokenserverclient.js
@@ +68,5 @@
> + *
> + *   malformedRequest -- If true, the server rejected the request because it
> + *     was invalid. If you see this, code in this file is likely wrong.
> + *
> + * If none of the above flags are set, there was a general server error.

It sounds like these four values are mutually exclusive, and could potentially grow in the future.

Why did you opt for this approach rather than a machine-processable "cause" field with a (future-proof) enumeration?

(Indeed, why did you opt for this rather than having a discrete set of *types*? Reading the comment on line 128 seems to suggest that approach.)

@@ +157,5 @@
> +   * `conditionsAccepted` argument to this function. Clients only need to set
> +   * this flag once per service and the server remembers acceptance.
> +   *
> +   * Clients should not blindly send acceptance to conditions. Instead, clients
> +   * should only set `conditionsAccepted` if and only if the server asks for

"should set … if and only if".

@@ +188,5 @@
>     *         (string) BrowserID assertion to exchange token for.
>     * @param  cb
>     *         (function) Callback to be invoked with result of operation.
> +   * @param  conditionsAccepted
> +   *         (bool) Whether to send acceptance to service conditions.

So what happens in the -- very rare -- situation that the list of conditions changes between a fetch and the user acceptance in the next request?

How do we denote which set of conditions was accepted? Is there an atomically incrementing version hiding in here somewhere?

@@ +317,5 @@
> +      // 403 should represent a "condition acceptance needed" response.
> +      //
> +      // The extra validation of "urls" is important. We don't want to signal
> +      // conditions required unless we are absolutely sure that is what the
> +      // server is asking for.

And yet you aren't validating the content of urls...
Attachment #652884 - Flags: review?(mconnor) → review?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #6)
> So what happens in the -- very rare -- situation that the list of conditions
> changes between a fetch and the user acceptance in the next request?
> 
> How do we denote which set of conditions was accepted? Is there an
> atomically incrementing version hiding in here somewhere?

There is no such facility on the server-side.  The database just has a boolean flag "accepted" that we flip on whenever we see the X-Conditions-Accepted header, and mass-reset to off if the terms ever change.
(In reply to Ryan Kelly [:rfkelly] from comment #7)

> There is no such facility on the server-side.  The database just has a
> boolean flag "accepted" that we flip on whenever we see the
> X-Conditions-Accepted header, and mass-reset to off if the terms ever change.

And that's a perfectly valid approach, so long as you know when you're setting the value that there's no race -- that is, when a user says "I agree" that the terms haven't changed since they retrieved them. It's the setting that needs the check.

The only way to do that is to have something richer than a boolean flag in the request that the client sends; the hash of the URLs, say, or an X-If-Unmodified-Since-esque value.

The alternative is that on Sunday I start the setup process; on Monday you change the terms; on Tuesday I finally click "I accept", and you have no standing whatsoever to determine whether I agreed to the current terms; only that I agreed to *some* terms.
Spun off Bug 785770 to track this, since it has the potential to get a bit rambling...
Depends on: 785770
(Assignee)

Comment 10

6 years ago
Created attachment 655833 [details] [diff] [review]
Add conditions acceptance to client, v3

I think I hit all points of feedback.
Attachment #652884 - Attachment is obsolete: true
Attachment #652884 - Flags: review?(rnewman)
Attachment #655833 - Flags: review?(rnewman)
Comment on attachment 655833 [details] [diff] [review]
Add conditions acceptance to client, v3

Review of attachment 655833 [details] [diff] [review]:
-----------------------------------------------------------------

One thing to guard against.

Marking this as r+, despite Bug 785770. You can either wait and address that, or land this and fix it later because it's not 'live'.

::: services/common/tokenserverclient.js
@@ +319,5 @@
> +
> +      let error = new TokenServerClientServerError();
> +      error.response = response;
> +
> +      if (response.status == 400) {

If this were a `switch`, you'd notice that you have the following exclusive cases:

400
401
403
404

If you get something else -- perhaps a 5xx -- you'll call cb() with an error object with no `cause` or `message` set. `undefined` probably isn't part of the strong `cause` enumeration!

I suggest setting `error.cause` on creation @ 321, or defaulting to "general" in the constructor.
Attachment #655833 - Flags: review?(rnewman) → review+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/services/services-central/rev/ed2b50cd3d66

Made "general" the default value for cause in the ctor.

We can address bug 785770 when it lands. I don't like keeping working patches around. In the mean time, this allows people to test things against the token server as it is deployed today.
Whiteboard: [fixed in services]
(In reply to Gregory Szorc [:gps] from comment #12)

> We can address bug 785770 when it lands. I don't like keeping working
> patches around. In the mean time, this allows people to test things against
> the token server as it is deployed today.

wfm.
Whiteboard: [fixed in services] → [fixed in services][qa-]
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3adff2d69d7

Meant to land this in inbound so others can use it sooner.
I live to set flags.
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/e3adff2d69d7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/ed2b50cd3d66
Whiteboard: [fixed in services][qa-] → [qa-]

Updated

5 years ago
Depends on: 927457
You need to log in before you can comment on or make changes to this bug.