Refactor basic, bearer, digest auth logic into separate class

[simov]

WIP

I moved the auth method logic into separate file. The auth method is split into two methods basic and bearer. Also I moved _digest and the part of the code in onRequestResponse related to these 3 methods.

The reasons to move exactly these functions is as follows: all private flags _hasAuth, _sentAuth and private variables _bearer,_user, _pass are local for the Auth class. That way request now knows only about only 3 distinct methods _auth.basic, _auth.bearer and _auth.response versus 6 variables _hasAuth, _sentAuth, _bearer, _user, _pass and _digest

The Auth class keeps a pointer to the request instance, but only use it to modify the headers, as well as reading some values (as I said private variables related to the Auth class are not set in request)

Other than that the code is mostly copy/pasted without changes, it certainly can be improved but I wanted to make this PR as short as possible. Also the debug call is currently commented out.

Ping @nylen @seanstrom @FredKSchott

[FredKSchott]

YESSS! I can't 👍 this enough!
Great idea, these kinds of projects are huge steps towards a more sane code base

[FredKSchott]

(I'm away for the long weekend so I can't review but from a quick readthrough it looks good

[nylen]

change _digest to digest to better match the naming of the other functions

[nylen]

This is excellent stuff @simov.

How about getting rid of any underscores in self._someValue, can't anything on the Auth object be a "public" property?

---

I'm not saying that we should do the following now. But, given that we want to move towards a composable API, what would it take to get there for this PR? I'm thinking that inside request.js, the code would look something like this:

require('./lib/auth').extend(Request)

What hooks would be necessary in request to make this work? Then, later on, we'd move request-auth into its own module and add some machinery for users to disable this default auth library.

One issue I foresee - I'm sure there are others: say an app has two different use cases for request where one needs to have the default auth provider and the other needs some custom thing, how do you accomplish this?

[nylen]

Oh and about the debug method, I think we could make it Request.prototype.debug without causing any issues. That would provide a natural way to call it here, and it would also get around the linter weirdness I experienced in #1354. I'll be glad to take that on after we merge this PR.

[simov]

I removed all occurances of _ in the Auth code.

@nylen I'm not sure what's your goal with this line in this case.

require('./lib/auth').extend(Request)

The idea of the Auth class is to have its own scope and instance to keep the auth related variables outside of Request. The methods in Auth are also meant to be accessed through self._auth.method in Request, instead of self.method

We can easily export Auth from request.js and then again from index.js so the user would be able to replace the Auth implementation like this request.Auth = require('other-auth') If we want this to be done for each instance of Request then we can attach it to the prototype Requst.prototype.Auth = Auth then initialize it in request.js with self._auth = new self.Auth(self)

About debug that would do the job 👍

[nylen]

@simov here's the idea: #1094 (comment)

Eventually, we would have request-base which contains the basic HTTP logic, probably including streaming too. It doesn't know anything about auth, but provides a set of hooks for other modules to implement auth, and request loads the base and extends it with commonly used features, something like this:

var request = require('request-base')
  , auth = require('request-auth')

auth.extend(request.Request)

Some of the benefits of a composable API like this:

• Modular code-base -> easier to understand
• People can extend or replace features in third-party libraries (in fact this is already happening to some extent, see request-promise, requestretry, request-debug). We could provide guidelines for people to implement and test plugins like this in a robust way.
• Porting to streams 2.0+ becomes very easy once most of the features are split out into separate modules: we just swap out request-base for something else.

My question was to try to get us to think through what those auth hooks would need to look like. We'd need a way to observe 401 responses and modify headers accordingly, and request-auth should probably add the Request.prototype.auth method itself. Other than that, I just wanted to continue the discussion from #1094.

Splitting code for features into separate files, like you've done here, is a necessary first step towards this approach, and we need to start pulling apart more features like this into separate layers. So I'll merge this in a couple of days, but please @FredKSchott @seanstrom and anyone else share your thoughts for where we want to go.

[simov]

I think we're looking too ahead of us atm, the functions shouldn't be allowed to explode into a couple of hundreds lines of code in the first place. The only way to really move something out of the code and stop thinking about it, is to make it properly encapsulated (of course without sacrificing too much while doing so). The rest is really just a matter of implementation preference in the consumer.

Take a look at my last commit. Now the Auth class is fully encapsulated, and it knows nothing about Request or its state, nor the heavily modified Response object state. That means the Auth class potentially can be re-used in different context. Also you can completely ignore it while looking at the Request code, because all that it affects is in the Request code itself (only the implementation is hidden which you certainly don't want to look at all the time).

---

About the plugin approach. There are three completely different things here:

1. Public chaining API the one attached on the prototype and accessed via request().auth
2. Options API the one passed as a map to request({options})
3. Internal hooks that request would use

Essentially what the extend approach suggests is this:

I wan't each module to extend the Request prototype and modify its public API

But why is that? Request should define it's own public API as a module, then use the hooks internally. Potentially we want to give the user ability to swap the internal implementation on the fly.

The extend techiquie implies that all of these custom 'modules' should work with heavily modified request and response instances, how exactly we're going to document that? It's like telling our users (developers/ourselfs) to take a look at a couple of hundreds lines of code in Request + in a few other 'modules' as well

Passing the request class in the extend method doesn't solve the problem when that module needs its own state, meaning that it's not just a utility module. So it must be instantiated inside the Request ctor.

Also the idea of having separate Auth class is to not pollute the Request state with methods and variables.

---

The Request.prototype.auth method itlsef is part of the public API of Request and I really don't like it.

We should almost always use a map for arguments, not only that it makes the invocation more readable, but you ca easily add just the needed parameters.

request.auth({bearer:'token'})

in place of

request.auth(null,null,false,'token')

just like it's used in the options object.

So we can implement support for a map args there, and leave the current implementation in Request as a backward compatibility.

[nylen]

Again I am totally satisfied with this PR for now and plan to merge it as-is. Encapsulation is a great first step and you've done a good job here.

We should almost always use a map for arguments, not only that it makes the invocation more readable, but you ca easily add just the needed parameters.

Agreed, and I'd like to start keeping a list of desired changes like this for our next major version. I have a couple too.

---

Request should define it's own public API as a module, then use the hooks internally.

-1. There is a lot of value in letting people mix and match request features. Our users are already doing this anyway with the libraries I listed above, so why not provide a well-documented way?

Here's another example: https://github.com/FrankyBoy/request-ntlm. NTLM auth would be nice to have (for those of us who get to deal with M$ stuff), but I doubt that any of us have time to do it, and that module should really be done as a wrapper around request instead of requiring a specific version of it.

Having an extend API or convention provides a nice path for extra features to become part of request core:

• Someone implements something cool like NTLM auth
• People test the library for a while outside of request, and we may point users to it as an unofficial solution
• Someone submits a patch to us with tests and docs, and ideally the patch to request is so simple that it's a pretty easy choice for us to merge it.

(ping @FrankyBoy - we're not to this point yet, but we're discussing how to do this at #1094 and here)

The extend techiquie implies that all of these custom 'modules' should work with heavily modified request and response instances, how exactly we're going to document that?

true, this is a concern. We will need to beef up our test suite as much as possible going forward.

Passing the request class in the extend method doesn't solve the problem when that module needs its own state, meaning that it's not just a utility module. So it must be instantiated inside the Request ctor.

Also the idea of having separate Auth class is to not pollute the Request state with methods and variables.

Agree with both of these points, that's the kind of thing I want to discuss here. Maybe request-auth does something like this:

exports.extend = function(Request) {
  Request._initHooks.push(function() {
    var self = this
    self._auth = new Auth()
  })
  Request.prototype.auth = function(options) {

  })
}

function Auth() {
  // ...
}

[simov]

Monkey patching have never been a problem in js. My whole point was that we should make sure we move as much code as cleanly as possible (capsulated), and then figure out what to do with the rest.

Because at the end of the day we're still going to support that code, even though it's in separate request- module, and non capsulated code ultimately leads to maintenance hell.

So I can live with that extend thing, as long as it doesn't modify the state, at least for those modules that live in the request org.

Other than that I'm glad we agree on most of the points here 👍 @nylen Also once this got it, I'll review my previous oauth refactoring PR, as there a few small things that I really don't like, and I'll make a new PR about it too.

[FredKSchott]

Instead of passing the raw headers, you could pass response.caseless and save yourself a second conversion step from headers -> caseless inside of auth.

[simov]

I intentionally made it that way, so that the module knows less about the consumer, and it can work with more generic input data parameters.

Also in caseless that's just an assignment, so I don't think it degrades performance or anything like that.

this.dict = dict || {}

[FredKSchott]

+1 lgtm

[simov]

@FredKSchott thanks for your feedback, I added the default values and the comment for the Auth class fields. See the comment about caseless above.

@nylen let me know if I have to squash the commits into one at some point.

[nylen]

yeah that would be good, if you squash this then I'll merge today. I was hoping some others would weigh in on our discussion but I guess not.

[simov]

Done

[FredKSchott]

@nylen seems like you guys covered most of it :)

Monkey patching isn't the answer imo. It might work for one or two plugins, but the conflicting code and assumptions of each plugin will become a nightmare at any sort of practical scale, especially if we see plugins becoming more prevalent once supported. I'd like to see us expose a collection of events or methods that a plugin can hook into.

Alternatively, we could actually define a plugin architecture for request to load:

plugin = {
  request:
  redirect: 
  response:
  // ...
}
request.loadPlugin(plugin);

This strategy has the additional benefit of supporting asynchronous plugins as well.

[FredKSchott]

I'm fine with anything we come up with, but -1 on blessing monkey patching as the official way to augment request.