Closed Bug 960878 Opened 10 years ago Closed 10 years ago

Allow /info/collections to accept expired tokens?

Categories

(Cloud Services Graveyard :: Server: Sync, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: rfkelly, Assigned: rfkelly)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file)

In a recent email thread, we discussed the possibility of allowing the /info/collections endpoint to accept expired tokenserver auth tokens.  This would allow use to optimize network use in the common case of there being nothing to sync.

In the current situation, you have to do this on pretty much every sync:

   1) ensure your FxA certificate is up-to-date, possibly fetching a fresh one from the fxa-auth-server
   2) fetch a fresh token from the TokenServer
   3) use the new token to hit /info/collections and see if you need to sync anything

If we allow the use of expired TS tokens, we can add a check and skip all of that:

  0) use existing token to hit /info/collections; if nothing has changed then go back to sleep

Shall we go ahead with this?

We'll need to update the sync1.5 protocol docs (Bug 951986) and implementation (Bug 960876) before launch, but we don't have to actually implement the client-side portion until later.
Summary: Allow /info/collections to accept expired tokens → Allow /info/collections to accept expired tokens?
There are actually two almost-orthogonal parts to this.

The first is Bug 736374, which is to short-cut i/c fetching itself: return a 304 if no collections have changed.

The second is to do one of two things. That there are two possibilities is why the two bugs are linked.

* Make only the 304 case less strongly authenticated. A client with an expired token will get a 304 or a 401.
* Make all cases less strongly authenticated. A client with an expired token will get a 304 or a 200.

Clients with new tokens will get a 304 or a 200 in either case, or only a 200 if Bug 736374 isn't implemented.


It's worth mentioning that expired tokens will only be permitted for a certain duration (12 hours?).
Depends on: 736374
> * Make only the 304 case less strongly authenticated. A client with an expired token will get a 304 or a 401.
> * Make all cases less strongly authenticated. A client with an expired token will get a 304 or a 200.

I strongly prefer the latter for both documentation and server-implementation simplicity.  But if it'll make or break the acceptability of this, either can be made to work without too much hassle.
(In reply to Ryan Kelly [:rfkelly] from comment #2)

> I strongly prefer the latter for both documentation and
> server-implementation simplicity.  But if it'll make or break the
> acceptability of this, either can be made to work without too much hassle.

Doesn't make-or-break it for me either way, but I figured it's worth declaring the minor information-leakage difference between the two.

Informally raising this choice with a representative of the privacy and/or sec teams sooner rather than later might save some effort.
> Informally raising this choice with a representative of the privacy
> and/or sec teams sooner rather than later might save some effort.

IIRC :telliott was going to send this upstream for comment
I was worried about it when there was a concern that you might not even need a token, but there's really no privacy leakage using expired tokens - you have to get the token in the first place, and we'll expire them after a few hours. From a privacy standpoint, it's the same as a longer-lived token.
Indeed.  What we're approximating here are tokens with multiple capabilities, where some of the capabilities expire much faster than the others.
(In reply to Toby Elliott [:telliott] from comment #5)
> I was worried about it when there was a concern that you might not even need
> a token, but there's really no privacy leakage using expired tokens - you
> have to get the token in the first place, and we'll expire them after a few
> hours. From a privacy standpoint, it's the same as a longer-lived token.

This is an excellent point.  Some of my initial push-back on this was thinking of the /i/c request as unauthenticated, but if we just accept expired tokens, that's fine.

Note to self and others: clients must still handle 401 from /i/c correctly; they can't assume that their expired-but-once-valid tokens will be good forever.  If the token server changes how it issues tokens, old tokens might be expunged.
If we're going to
Whiteboard: [qa+]
nm.
My one lingering concern here is how this will affect node-reassignment.  AFAICT this will slow-down the detection of a node-reassignment until either (a) you have data to push up to sync, or (b) your expired token eventually times out even for /info/collections.  I guess we would just need to pick a sensible value for that eventual timeout.

We could also make an explicit "sync now" always fetch a fresh token, so that if a pending node-reassignment is causing sync to appear not to work, this will kick things back into shape.
On the plus side, it totally avoids node reassignment write races :P
OK: https://github.com/mozilla-services/docs/commit/a5fa008b767c6ac02d7025e6b3fb1dc34d6311a7

Can we get away with being this non-committal at the protocol level?
I think so. The server should always reserve the right to authenticate every request, so this seems factual to me.
Blocks: 968592
Priority: -- → P2
/cc Mark to put this on his radar for desktop.

We haven't implemented this yet on the server, but we do plan to.  The biggest performance gain will probably be seen from desktop devices with no other devices attached, since it's basically a shortcut for detecting that "nothing has changed".
This probably isn't going to be trivial for desktop.  We currently check token expiry and fetch a new one when appropriate - and we don't have the context to know what the token will be used for.

It *might* be easier for us to refactor things so we assume tokens never expire, and just handle a 401 and retry the request - currently we handle the 401, but don't retry.  We already have another work-around in place because of this (IIRC, we actually assume a lifetime of only 80% of the actual lifetime to try and avoid a token expiring in the time between we check and the time it actually gets passed to the server, so this would help avoid that.)

Chris, any thoughts?
> It *might* be easier for us to refactor things so we assume tokens never expire, and just handle a 401 and retry the request - currently we handle the 401, but don't retry.  We already have another work-around in place because of this (IIRC, we actually assume a lifetime of only 80% of the actual lifetime to try and avoid a token expiring in the time between we check and the time it actually gets passed to the server, so this would help avoid that.)

This is possible. We do actually handle an expired token error, but I didn't figure out how to prevent it from showing the nasty bottom bar when that happens. 401s were rare in Legacy sync but possibly routine FxA Sync. I have it fetch a new token if it's 80% through its lifetime to prevent that bar from showing. Maybe Richard can help with that next week.
(In reply to Mark Hammond [:markh] from comment #15)
> This probably isn't going to be trivial for desktop.  We currently check
> token expiry and fetch a new one when appropriate - and we don't have the
> context to know what the token will be used for.

You could use a different authenticator for the info/collections fetch. This bound authenticator would have a different hasValidToken method.

You might need to reorder the startup process to avoid pre-emptively fetching a token, but that should be reasonably straightforward.

Much of the benefit of this approach applies if you can persist tokens, by the way.

There will be edge cases, of course!
Here's the server-side implementation for this functionality.

It customizes the shared TokenServerAuthenticationPolicy class to handle expired tokens.  To help avoid future footguns, it gives expired-but-otherwise-valid token holders a different "effective principal" than holders of proper valid tokens.  Basically their username appears to be "expired:foo" rathern than just "foo".  The /info/collections view then gets a special ACL that allows this username access to just that view.

I'm not thrilled to have re-implemented most of TokenServerAuthenticationPolicy.decode_hawk_id, but I don't see a clean way to refactor the superclass to support this functionality while keeping it totally optional.
Assignee: nobody → rfkelly
Attachment #8395506 - Flags: review?(telliott)
Attachment #8395506 - Flags: review?(telliott) → review+
The direct scope of this bug is now done: "allow /info/collections to accept expired tokens".  It will be allowed as of the next deployment, which is tracked in Bug 987455.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Verified here: https://docs.services.mozilla.com/storage/apis-1.5.html
and here: https://github.com/mozilla-services/server-syncstorage
and via latest unit tests...
Status: RESOLVED → VERIFIED
Product: Cloud Services → Cloud Services Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: