The AitC client needs MAC authenticated HTTP requests, but this functionality might be useful to other code too.

This patch provides the AuthRESTRequest which takes a token and allows the caller to make MAC authenticated HTTP requests. The patch assumes that the common CryptoUtils from bug 745396 are available.

Comment on attachment 615013 [details] [diff] [review] Provides a AuthRESTRequest object - v1 Review of attachment 615013 [details] [diff] [review]: ----------------------------------------------------------------- I agree with the general direction of the patch. But, I'm cancelling review. The main reason is there are no tests. Always include tests. For this one, I'm fine with a test that runs a local HTTP server or one that monkeypatches RESTRequest.prototype.dispatch and verifies the header is being set accordingly. I'd prefer an HTTP server, as I like end-to-end tests. Also, please add the new type to EXPORTED_SYMBOLS at the top of the file (if you had a test, you'd realize this new type is inaccessible outside of the file). ::: services/common/rest.js @@ +506,5 @@ > + * the string is not a valid URI. > + * @param authToken > + * An auth token of the form {id: (string), key: (string)} from > + * which the MAC Authentication header for this request will be > + * derived. Please add a note that the output from TokenServerClient.getToken*() is a suitable input. @@ +508,5 @@ > + * An auth token of the form {id: (string), key: (string)} from > + * which the MAC Authentication header for this request will be > + * derived. > + */ > +function AuthRESTRequest(uri, authToken) { "Auth" is too generic. How about TokenAuthenticatedRequest? @@ +516,5 @@ > +AuthRESTRequest.prototype = { > + // This object extends RESTRequest and override the internal dispatch > + // method to add the MAC Authentication header. > + __proto__: RESTRequest.prototype, > + dispatch: function(method, data, onComplete, onProgress) { Insert empty line between. Name the function to match property name. @@ +522,5 @@ > + this.authToken.id, > + this.authToken.key, > + method, > + this.uri, > + {} Please support "extra" argument. This would be a follow-up bug, so best to support it now. FWIW, if there are no extra arguments, you don't need to pass the argument. @@ +525,5 @@ > + this.uri, > + {} > + ); > + this.setHeader("Authorization", sig.getHeader()); > + this._log.info("Complete AUTHRESTrequest: " + method + " " + this.uri.asciiSpec); This is already logged by the parent (albeit at Trace level). Get rid of it. @@ +532,5 @@ > + RESTRequest.prototype.dispatch.call(this, method, data, function (error) { > + self._log.info( > + "AUTHRestRequest result: " + this.uri.asciiSpec + > + " status: " + this.response.status > + ); This is also logged by the parent. Nuke this function. @@ +536,5 @@ > + ); > + onComplete(error); > + }, onProgress); > + } > +}; Please move the block to after RESTResponse. I'm tempted to ask for a new file, but that is probably overkill right now.

Now with 100% more XPCShell test goodness (it passes)!

Note: you need the patch in bug 745396 applied for this to work. Consequently, I will hold off on pushing to try until that crypto/utils refactoring lands.

Comment on attachment 615085 [details] [diff] [review] Provides a AuthRESTRequest object - v2 Review of attachment 615085 [details] [diff] [review]: ----------------------------------------------------------------- This patch just has little nits, so granting r+. I would still like explicit review on the head_global.js changes, so please submit the next patch for review before commit. I will try to prod rnewman to get the crypto patch reviewed before EOD. ::: services/common/rest.js @@ +616,5 @@ > + this.authToken.key, > + method, > + this.uri, > + this.extra > + ); While I personally prefer this style for calling functions with many arguments, our style is to wrap like: let sig = CryptoUtils.computeHTTPMACSHA1(this.authToken.id, this.authToken.key, method, this.uri, this.extra); Will Bugzilla preserve formatting? I don't know. "method" is supposed to be directly under the beginning of "this.authToken" @@ +623,5 @@ > + this.setHeader("Authorization", header); > + > + RESTRequest.prototype.dispatch.call( > + this, method, data, onComplete, onProgress > + ); I think this line is exactly 80 chars. Please wrap. (We don't count the newline against the 80 char limit.) ::: services/common/tests/unit/test_tokenauthenticatedrequest.js @@ +8,5 @@ > + let uri = Services.io.newURI("resource:///modules/services-crypto/", > + null, null); > + resProt.setSubstitution("services-crypto", uri); > +} > +addCryptoResourceAlias(); We already have something similar in services/common/tests/unit/head_global.js. Please update addResourceAlias() there to also register services-crypto. @@ +28,5 @@ > + > + let id = "eyJleHBpcmVzIjogMTM2NTAxMDg5OC4x"; > + let key = "qTZf4ZFpAMpMoeSsX3zVRjiqmNs="; > + let method = "GET"; > + let uri = Services.io.newURI(TEST_SERVER_URL + "foo", null, null); Nit: Use CommonUtils.makeURI() @@ +31,5 @@ > + let method = "GET"; > + let uri = Services.io.newURI(TEST_SERVER_URL + "foo", null, null); > + let nonce = btoa(CryptoUtils.generateRandomBytes(16)); > + let ts = Math.floor(Date.now() / 1000); > + let extra = { ts: ts, nonce: nonce, ext: message }; Nit: braces should be cuddled. e.g. {ts: ts Also, extra is either going to work or it isn't. I'd populate only one field instead of doing extra work. KISS. @@ +46,5 @@ > + } > + }); > + > + let req = new TokenAuthenticatedRESTRequest( > + uri, { id: id, key: key }, extra Nit: Cuddle against braces. This also reminds me that I need to implement the testing infrastructure so you can actually call TokenServerClient.getTokenFromBrowserIDAssertion() and thus perform an actual end-to-end test with the appropriate APIs. Please add a TODO and reference bug 745800.

(In reply to Gregory Szorc [:gps] from comment #5) > ::: services/common/tests/unit/test_tokenauthenticatedrequest.js > @@ +8,5 @@ > > + let uri = Services.io.newURI("resource:///modules/services-crypto/", > > + null, null); > > + resProt.setSubstitution("services-crypto", uri); > > +} > > +addCryptoResourceAlias(); > > We already have something similar in > services/common/tests/unit/head_global.js. Please update addResourceAlias() > there to also register services-crypto. That's where I originally put it, but then moved it to the test file because it is the only one that depends on crypto modules. If we feel like there might be subsequent tests in this set that will rely on it, putting it in global makes sense.

(In reply to Gregory Szorc [:gps] from comment #5) > ::: services/common/rest.js > @@ +616,5 @@ > > + this.authToken.key, > > While I personally prefer this style for calling functions with many > arguments, our style is to wrap like: I can't fit the second argument into the first line within 80 chars, so I just moved all arguments to the next line. > @@ +623,5 @@ > > + this.setHeader("Authorization", header); > > + > > + RESTRequest.prototype.dispatch.call( > > + this, method, data, onComplete, onProgress > > + ); > > I think this line is exactly 80 chars. Please wrap. (We don't count the > newline against the 80 char limit.) It exceeds 80 chars even excluding the newline (or my editor is lying to me!), so I left it as it is. > Also, extra is either going to work or it isn't. I'd populate only one field > instead of doing extra work. KISS. Both timestamp and nonce are required to make the test work, I removed the message. Fixed all the other nits!

Comment on attachment 615459 [details] [diff] [review] Provides a AuthRESTRequest object - v2.1 Review of attachment 615459 [details] [diff] [review]: ----------------------------------------------------------------- What happened to the test file? ::: services/common/tests/unit/head_global.js @@ +50,5 @@ > let uri = Services.io.newURI("resource:///modules/services-common/", null, > null); > handler.setSubstitution("services-common", uri); > + > + uri = Services.io.newURI("resource:///modules/services-crypto/", null, Nit: use let (Yes, you are allowed to declare the same variable multiple times in the same scope. I didn't realize this either until a short while ago.)

Gah, forgot to hg add before diffing.

Fixed argument alignment.

I r+'d a slightly modified patch during a pair programming session.

Comment on attachment 615576 [details] Provides a AuthRESTRequest object - v2.2.1 Similar patch was reviewed offline.

[qa-] but I will be smoketesting setup and Sync on s-c to be certain nothing is broken.