Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

GET /account/devices should not return session token ids #1119

Closed
philbooth opened this issue Nov 19, 2015 · 4 comments
Closed

GET /account/devices should not return session token ids #1119

philbooth opened this issue Nov 19, 2015 · 4 comments
Assignees

Comments

@philbooth
Copy link
Contributor

Another day, another stupid problem with my implementation of the devices API. :)

Currently the /v1/account/devices endpoint returns a property sessionToken which contains the token id. But API clients don't know what their token id is, so this information is useless and they can't identify themselves in the returned array.

I'm pretty sure it was suggested at some point that I return an isCurrentDevice boolean property instead, so that is probably the correct thing to do.

It also highlights another gap in the test coverage as I wasn't asserting that the returned sessionToken property was doing what I thought it did. The fix for this issue should add appropriate assertions to test/remote/devices.js.

We also need to update the docs and the client code.

@rfk
Copy link
Contributor

rfk commented Nov 19, 2015

But API clients don't know what their token id is

It's been a while since I was deep down in the mechanics of our session tokens - why do they not know this?

@philbooth
Copy link
Contributor Author

why do they not know this?

I'm not sure I can answer why. But I might be operating on the wrong premise here so if I describe things as far as I understand them, perhaps you can set me straight. Fwiw, the session auth stuff is still basically magic to me, I'll dig into it some more tomorrow.

Anyway, my understanding:

  • The auth server returns sessionTokens.tokenData (among other things) after sign-in and that field is the thing that clients call a session token.
  • The thing that we use to identify session tokens in the auth server, sessionTokens.tokenId, is not exposed.
  • We could modify /account/devices so that it sets sessionToken to tokenData instead of tokenId, but doing so would require either a database migration or an extra query. Neither of those seems like a great option.
  • The only reason for returning a sessionToken from /account/devices at all was so that the content server could identify itself in the response array. Switching to a boolean enables that.

@rfk
Copy link
Contributor

rfk commented Nov 19, 2015

We could modify /account/devices so that it sets sessionToken to tokenData instead of tokenId

We should avoid this; the tokenId is enough to uniquely identify a session but does not give the holder power to actually use that session, while tokenData does grant such power. I'd not be comfortable handing that out in API responses, even if authenticated to some other device.

FWIW, you can derive tokenId from tokenData by applying some hashing, so clients could in theory figure out their tokenId. But they shouldn't have to if we can just tell them "hey, this is you".

The only reason for returning a sessionToken from /account/devices at all was so that the
content server could identify itself in the response array.

Another potential reason for returning it, is to identify the device(s) in a broader list of sessions. Since some sessions have devices and some don't, we talked about the "devices view" doing something like:

  • Fetch the list of all active session tokens, be they devices or not
  • Fetch the list of devices, and match them up with items from the previous list
  • Show detailed name/type info about the ones that are devices, and more generic info about the sessions that don't have a device record

But there's other ways to achieve, and it's not clear if we actually need it, so shrug.

@rfk
Copy link
Contributor

rfk commented Nov 20, 2015

shrug

Which is to say, I think we'll continue to evolve the API on this point, but your proposal sounds good to me at this time :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants