feat(clients): implement all the commands #4

seanmonstar · 2014-11-19T22:45:35Z

This command will return a list of clients registered with the server.
The command can be passed a --token option to skip fetching a new token.

rfk · 2014-11-20T02:30:50Z

bin/fxa-oauth.js

-      password: password,
+  error.assert(program.fxa, '--fxa or --env');
+  error.assert(program.url, '--url or --env');
+  if (program.token) {


Do you need to add some sort of program.option('--token <token') declaration in the option-parsing logic to accommodate this?

I fear that this command-line-parsing lib doesn't like having both a --token option and a token sub-command.

When I try to run it like this to generate an oauth token:

./bin/fxa-oauth.js --env=latest --user=rfkelly@mozilla.com token 24bdbfa45cd300c5 oauth

It fails out complaining about a "missing email". Tracing it through, I can see that it's taking this if (program.token) branch even though I haven't specified a --token command-line option. And logging the contents of program.token show that it's some sort of object with the command-line args etc in it.

Actually this is better as it's a valid canGrant clientId:

./bin/fxa-oauth.js --env=latest --user=rfkelly@mozilla.com token 5882386c6d801776 oauth

This works on master but fails out with "missing email" on this branch.

It also works if I rename the --token option to --tok.

seanmonstar · 2014-12-09T23:42:44Z

@rfk forgot to mention last week that i'd pushed an update.

rfk · 2014-12-10T00:40:59Z

bin/fxa-oauth.js

@@ -40,6 +43,15 @@ const URLS = {
  ]
 };

+function yesno(val) {


The name suggests this should handle strings like "no" and "false" as well, but it doesn't. Should it?

I'd also prefer a clearer name e.g. toBoolean, but don't feel strongly about it.

this should handle strings like "no" and "false" as well

Uh, my bad, of course it handles this as part of the "anything that's not explicitly true" default case.

rfk · 2014-12-10T00:48:11Z

Code looks good, I'm going to try taking it for a spin.

rfk · 2014-12-10T00:59:50Z

bin/fxa-oauth.js

 const Client = require('../');
 const log = require('npmlog');
 const read = P.promisify(require('read'));

+const CLI_CLIENT_ID = '66041b7ec3991ec0';


Is this a newly-provisioned ID that we need to add to all our envs?

seanmonstar · 2015-01-21T01:23:42Z

README.md

@@ -3,3 +3,26 @@
 A command line tool to ease interacting with the OAuth implementation of
 Firefox Accounts.

+```


@ckolos How's this usage look?

Gimme a sample usage and lgtm...

seanmonstar · 2015-01-21T01:24:36Z

@rfk ok, so I removed the token option, since it seemed problematic. Instead, the Client will just fetch a token for every request, and then cleanup afterwards.

seanmonstar · 2015-01-21T01:27:51Z

lib/index.js

+      method: 'post',
+      url: this._baseUrl + '/v1/destroy',
+      json: {
+        token: token


This depends on mozilla/fxa-oauth-server#198

rfk · 2015-01-27T02:02:48Z

Code looks good, will take it for a spin this afternoon

rfk · 2015-01-27T04:01:11Z

README.md

+    -u, --user <email>  Env: FXA_USER
+    --url <url>         The base url of the OAuth server
+    --fxa <url>         The base url of the Auth server
+    -v, --verbose       Receive verbose output.


We might consider a matching -q, --quiet option as well; the default logging verbosity seems to log all requests and responses, which is nice for trying it out but may be too verbose for e.g. scripted use.

rfk · 2015-01-27T04:04:07Z

Running the clients command I get an error like so:

fxa http 403 https://oauth-latest.dev.lcip.org/v1/clients
fxa ERR! oauth Forbidden
fxa ERR! token temporary token was not cleaned up!

I guess this is because of mozilla/fxa-oauth-server#198 which hasn't propagated out to the dev servers yet?

rfk · 2015-01-27T04:23:36Z

bin/fxa-oauth.js

+    name: {},
+    redirectUri: {},
+    imageUri: {},
+    canGrant: {


These all need to be snake_case rather than camelCase, to match the form expected by the API per https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#post-v1client

rfk · 2015-01-27T04:31:18Z

After the above-mentioned tweaks to get the register command running, it fails with a 500 Server Error and the following traceback on the fxa-oauth-server logs:

{"Timestamp":1422332816497000000,"Logger":"fxa-oauth-server","Type":"summary.summary","Severity":2,"Pid":13970,"EnvVersion":"2.0","Fields":{"code":500,"errno":999,"method":"post","path":"/v1/client","t":21,"auth":"{\"user\":\"1c44d32a121546d8a5b3431dab450cfb\",\"scope\":[\"oauth\"]}","payload":"[\"name\",\"can_grant\",\"image_uri\",\"redirect_uri\"]","stack":"Error: whitelisted is required\n    at Object.exports.create (/data/fxa-oauth-server/node_modules/hapi/node_modules/boom/lib/index.js:21:17)\n    at Object.exports.internal (/data/fxa-oauth-server/node_modules/hapi/node_modules/boom/lib/index.js:246:92)\n    at Object.exports.badImplementation (/data/fxa-oauth-server/node_modules/hapi/node_modules/boom/lib/index.js:282:23)\n    at postValidate (/data/fxa-oauth-server/node_modules/hapi/lib/validation.js:150:26)\n    at internals.Any._validateWithOptions (/data/fxa-oauth-server/node_modules/joi/lib/any.js:430:16)\n    at root.validate (/data/fxa-oauth-server/node_modules/joi/lib/index.js:102:23)\n    at exports.response (/data/fxa-oauth-server/node_modules/hapi/lib/validation.js:159:16)\n    at /data/fxa-oauth-server/node_modules/hapi/lib/request.js:304:13\n    at iterate (/data/fxa-oauth-server/node_modules/hapi/node_modules/items/lib/index.js:35:13)\n    at done (/data/fxa-oauth-server/node_modules/hapi/node_modules/items/lib/index.js:27:25)"}}

I guess this is due to not providing a value for whitelisted as part of the request. Forcing it to send whitelisted=false allows the registration to succeed for me.

rfk · 2015-01-27T05:02:36Z

bin/fxa-oauth.js

+  .command('update <clientId> <property> <value>')
+  .description('Update a property of a client.')
+  .action(function(id, prop, value) {
+    //TODO: validate id,prop,value


I tried update CLIENTID canGrant true and it reported success, despite the property name being invalid. I guess this is partly missing client-side validation, and partly the server not rejecting unknown fields.

Doing update CLIENTID can_grant true seems to work correctly, with the modified value reflecting in a subsequent client listing.

I'm thinking that the leniency allowed in the server to ease using the OAuth endpoints should only be enabled for those specific endpoints (POST /v1/authorization). Otherwise, tossing 400s seems better.

- `list`: provides a list of all registered clients - `register`: prompts for details to register a new client - `update`: updates a single property on a registered client - `delete`: deletes a client! boom! The commands will delete the token after usage, or blow up with an error if the token couldn't be deleted.

seanmonstar · 2015-02-02T19:26:29Z

Addressed comments.

ckarlof · 2015-02-03T23:24:24Z

@rfk, after @seanmonstar's most recent updates, are we ready to merge?

rfk · 2015-02-03T23:24:56Z

I am literally trying this out again right now :-)

rfk · 2015-02-04T00:14:38Z

bin/fxa-oauth.js

+  .action(function(id, prop, value) {
+    //TODO: validate id,prop,value
+    withTempToken(function(oauth) {
+      var client = { id: id };


The oauth server doesn't take "id" in the request body when updating a client record. This used to be silently ignored but it's now failing. We need to either accept a matching "id" in the request body on the server, or stop sending it in the client.

rfk · 2015-02-04T00:21:25Z

There's something weird going on with the temp-token cleanup and the delete command. Take a look at the following:

$> ./bin/fxa-oauth.js -e latest delete 061b0de00efa3424 
fxa info env latest
fxa info user rfkelly@mozilla.com
fxa http POST https://latest.dev.lcip.org/auth/v1/account/login
fxa http 200 https://latest.dev.lcip.org/auth/v1/account/login
fxa http POST https://latest.dev.lcip.org/auth/v1/certificate/sign
fxa http 200 https://latest.dev.lcip.org/auth/v1/certificate/sign
fxa http POST https://oauth-latest.dev.lcip.org/v1/authorization
fxa http 200 https://oauth-latest.dev.lcip.org/v1/authorization
fxa http GET https://oauth-latest.dev.lcip.org/v1/client/061b0de00efa3424
fxa http POST https://oauth-latest.dev.lcip.org/v1/destroy
fxa http 200 https://oauth-latest.dev.lcip.org/v1/client/061b0de00efa3424
Delete  "{\"id\":\"061b0de00efa3424\",\"name\":\"rfkelly test client four\",\"image_uri\":\"\",\"redirect_uri\":\"http://www.rfk.id.au/tst/me\"}"
Are you sure? y/n? (n) fxa http 200 https://oauth-latest.dev.lcip.org/v1/destroy
y
fxa info delete-client yes
fxa http DELETE https://oauth-latest.dev.lcip.org/v1/client/061b0de00efa3424
fxa http 401 https://oauth-latest.dev.lcip.org/v1/client/061b0de00efa3424

AFAICT what's happening is, it stops to prompt me and confirm the client deletion, but the cleanup code executes without waiting for my reply. The token gets deleted, then a enter "yes", and it gets a 401 trying to use the now-deleted token.

rfk · 2015-02-04T00:22:12Z

(I've also been getting sporadic 401s with other commands, which might be the same issue, but not surfacing regularly because they don't stop and prompt for any input)

rfk · 2015-02-04T01:02:21Z

With the client-id-field and switch-done-to-then fixes mentioned above, all commands run successfully for me; r+ with those fixes

seanmonstar · 2015-02-04T02:19:58Z

@rfk your thorough reviewing has been invaluable. I've addressed the latest comments, in a new commit to make it easier to see what changed.

rfk · 2015-02-04T02:24:17Z

nice work @seanmonstar, 👍

feat(clients): implement all the commands

rfk reviewed Nov 20, 2014
View reviewed changes

seanmonstar force-pushed the list branch from 7f05869 to bc3dd64 Compare December 2, 2014 03:46

rfk reviewed Dec 10, 2014
View reviewed changes

rfk added the waffle:in progress label Dec 11, 2014

seanmonstar changed the title ~~feat(clients): implement the "clients" command~~ feat(clients): implement all the commands Jan 6, 2015

seanmonstar force-pushed the list branch 2 times, most recently from 3b18e88 to e079ebe Compare January 21, 2015 01:22

seanmonstar reviewed Jan 21, 2015
View reviewed changes

rfk reviewed Jan 27, 2015
View reviewed changes

rfk mentioned this pull request Jan 27, 2015

500 server error when whitelisted is not passed to POST /client mozilla/fxa-oauth-server#203

Closed

rfk reviewed Jan 27, 2015
View reviewed changes

seanmonstar force-pushed the list branch from e079ebe to 691419e Compare February 2, 2015 19:26

rfk reviewed Feb 4, 2015
View reviewed changes

review comments

572a81c

rfk added a commit that referenced this pull request Feb 4, 2015

Merge pull request #4 from mozilla/list

14c4db2

feat(clients): implement all the commands

rfk merged commit 14c4db2 into master Feb 4, 2015

rfk removed the waffle:in progress label Feb 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clients): implement all the commands #4

feat(clients): implement all the commands #4

seanmonstar commented Nov 19, 2014

rfk Nov 20, 2014

rfk Dec 10, 2014

rfk Dec 10, 2014

rfk Dec 10, 2014

seanmonstar commented Dec 9, 2014

rfk Dec 10, 2014

rfk Dec 10, 2014

rfk commented Dec 10, 2014

rfk Dec 10, 2014

seanmonstar Jan 21, 2015

ckolos Jan 21, 2015

seanmonstar commented Jan 21, 2015

seanmonstar Jan 21, 2015

rfk commented Jan 27, 2015

rfk Jan 27, 2015

rfk commented Jan 27, 2015

rfk Jan 27, 2015

rfk commented Jan 27, 2015

rfk Jan 27, 2015

seanmonstar Feb 2, 2015

seanmonstar commented Feb 2, 2015

ckarlof commented Feb 3, 2015

rfk commented Feb 3, 2015

rfk Feb 4, 2015

rfk commented Feb 4, 2015

rfk commented Feb 4, 2015

rfk commented Feb 4, 2015

seanmonstar commented Feb 4, 2015

rfk commented Feb 4, 2015

feat(clients): implement all the commands #4

feat(clients): implement all the commands #4

Conversation

seanmonstar commented Nov 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar commented Dec 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar commented Jan 21, 2015

Choose a reason for hiding this comment

rfk commented Jan 27, 2015

Choose a reason for hiding this comment

rfk commented Jan 27, 2015

Choose a reason for hiding this comment

rfk commented Jan 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar commented Feb 2, 2015

ckarlof commented Feb 3, 2015

rfk commented Feb 3, 2015

Choose a reason for hiding this comment

rfk commented Feb 4, 2015

rfk commented Feb 4, 2015

rfk commented Feb 4, 2015

seanmonstar commented Feb 4, 2015

rfk commented Feb 4, 2015