Add support for "device commands" feature. #2449
Conversation
ghost
assigned
| with additional event-specific data. | ||
|
|
||
| The currently supported commands are: | ||
|
|
eoger
May 22, 2018
Contributor
Should we also talk about the payload-less command that means "account/session verified"?
Should we also talk about the payload-less command that means "account/session verified"?
rfk
force-pushed the
feature.pushbox-ng
branch
18 times, most recently
from
82cb0a0
to
1da476f
| @@ -85,7 +85,7 @@ | |||
| "acorn": "5.0.3", | |||
| "commander": "2.9.0", | |||
| "eslint-plugin-fxa": "git+https://github.com/mozilla/eslint-plugin-fxa#master", | |||
| "fxa-auth-db-mysql": "git+https://github.com/mozilla/fxa-auth-db-mysql.git#master", | |||
| "fxa-auth-db-mysql": "git+https://github.com/mozilla/fxa-auth-db-mysql.git#feature.pushbox-ng", | |||
rfk
May 29, 2018
Author
Member
We'll need to revert this before merge.
We'll need to revert this before merge.
| @@ -101,8 +101,6 @@ describe('remote session', function() { | |||
| }) | |||
| .then(() => { | |||
| return client.api.sessionStatus(sessionTokenCreate) | |||
| }, () => { | |||
| assert(false, 'failed to destroy the session') | |||
rfk
May 29, 2018
Author
Member
This was unnecessary given that we catch and check the error below.
This was unnecessary given that we catch and check the error below.
rfk
changed the title
|
Aha, my regex was not allowing the "." character in this string. If you use a short name like "open-uri" for testing then it should work. I'll see about getting an update pushed on the rebuilt box shortly. |
rfk
force-pushed the
feature.pushbox-ng
branch
4 times, most recently
from
cd96491
to
1cbe3fe
|
So, we've had this up and running for a while on https://pushbox2.dev.lcip.org and it seems to be working as intended. @eoger was able to point a desktop implementation of send-tab at it and things worked correctly. I've done another pass, fleshed out the docs and addressed a few nits, so I think it's ready for a final r? @eoger, do you have any final commentary or changes to the naming here based on your client implementation? In particular I'm wondering about changing "messages" to "commands" in the response for /account/device/commands, but any other feedback before we land will be valuable. |
| The time in milliseconds after which the command should expire, | ||
| if not processed by the device. | ||
|
|
||
| <!--end-request-body-post-accountdevicesinvoke_command-ttl--> |
philbooth
Jun 20, 2018
Contributor
Excellent docs, thanks!
Excellent docs, thanks!
| url: { | ||
| doc: 'Pushbox Server URL', | ||
| format: 'url', | ||
| default: 'http://localhost:8057/', |
shane-tomlinson
Jun 20, 2018
Member
Do we still default to production values? Is this the production value?
Do we still default to production values? Is this the production value?
rfk
Jun 20, 2018
Author
Member
You're right we should, but we don't (yet) have a production value until the service gets deployed. I'll find out if that's been decided yet. Also note that we land it preffed off by default.
You're right we should, but we don't (yet) have a production value until the service gets deployed. I'll find out if that's been decided yet. Also note that we land it preffed off by default.
| @@ -278,6 +280,8 @@ for `code` and `errno` are: | |||
| A TOTP token not found. | |||
| * `code: 400, errno: 156`: | |||
| Recovery code not found. | |||
| * `code: 400, errno: 157`: | |||
shane-tomlinson
Jun 20, 2018
Member
Is this errno ever surfaced to the content server? If so, we'll need to add an entry for it.
Is this errno ever surfaced to the content server? If so, we'll need to add an entry for it.
rfk
Jun 20, 2018
Author
Member
The content-server should never see an "unavailable device command" error unless it tries to invoke a device command for itself, which I don't think we've got plans to do anytime soon.
The content-server should never see an "unavailable device command" error unless it tries to invoke a device command for itself, which I don't think we've got plans to do anytime soon.
vbudhram
Jun 20, 2018
Contributor
@shane-tomlinson I misread your comment as recovery code was not surfaced. I'll keep the linked bug open since we could do a little cleanup there.
@shane-tomlinson I misread your comment as recovery code was not surfaced. I'll keep the linked bug open since we could do a little cleanup there.
| The device may wish to update | ||
| any cached list of other connected devices. | ||
| * `fxaccounts:device_disconnected`: | ||
| A device has been disconnected to the account. |
shane-tomlinson
Jun 20, 2018
Member
to the account => from the account
to the account => from the account
| ## Device Commands | ||
|
|
||
| Connected devices may be able to | ||
| offer functionality to one other |
shane-tomlinson
Jun 20, 2018
Member
to one other or one another?
to one other or one another?
| const mergedInfo = Object.assign({}, device, token) | ||
| return { | ||
| id: device.id, | ||
| sessionToken: device.sessionTokenId, |
shane-tomlinson
Jun 20, 2018
Member
Shouldn't sessionTokenId already be in mergedInfo? I'm confused why mergedInfo is created and then some fields are fetched from device instead.
Shouldn't sessionTokenId already be in mergedInfo? I'm confused why mergedInfo is created and then some fields are fetched from device instead.
rfk
Jun 20, 2018
Author
Member
I don't think there's any particular reason, it's just that we know some of those keys are only on the device that we read from the db. I'll make them all mergedInfo for consistency, assuming that doesn't surprise me with any test bustage.
I don't think there's any particular reason, it's just that we know some of those keys are only on the device that we read from the db. I'll make them all mergedInfo for consistency, assuming that doesn't surprise me with any test bustage.
rfk
force-pushed the
feature.pushbox-ng
branch
2 times, most recently
from
fae3465
to
5b455f7
|
I read this a second time, and also worked with that code on my desktop patch. I'm pretty happy with what we have here, I think we should merge. |
|
OFFICIAL r+ |
|
@rfk - the api.md file looks like a trivial merge conflict, but I don't want to fix & merge in case you have additional changes you'd like to make. Can you merge when ready so we can cut the train? |
|
Thanks @PB @shane-tomlinson, rebased on latest master and will merge when green. |
|
@rfk seems like tests failed |
|
Yeah, will dig in after meetings :-( |
^ This needs to use a |
|
Thanks @vladikoff, looks like that fixed it. |
This is an extension of and alternative to the "device messages" feature from #2328, that's designed to support the latest requirements of new-send-tab while being a step along to road to a more general peer discovery API.
As part of its device registration, a device can include a set of "supported commands". This is a simple mapping where the keys are string URIs naming commands that the device can be asked to perform, and the values are additional metadata necessary in order to invoke the command. Other devices can query the device list to discover their peers and the commands that they support, and can invoke a command on another device by issuing a
POSTrequest.A good place to start is the new device registration documentation that's included as part of this PR. This feature branch is also deployed to https://pushbox2.dev.lcip.org if you'd like to poke at the API live.
Connects to #2318