Refactor of oauth package

[ghost]

The oauth package has grown enough to become a mess. I think that we should invest some time to refactor it. Seeing how it is used by many other packages, I don't believe this will be a one-step process. This is why I am creating this issue where the general direction can be discussed and implemented through a series of pull-requests.

Currently, the export section of oauth looks like this:
https://github.com/cloudfoundry-incubator/cf-abacus/blob/ab1b40a2c0eabc0634147a227783743f71f08482/lib/utils/oauth/src/index.js#L492-L501

The functions there are various in nature: from middlewares to assertion functions; from basic-authentication ones to bearer-token ones; from request manipulation to header manipulation to direct token manipulation; all the way to generic tokens.
Not to mention that there is code (basicStrategy) that is used only by the cf-abacus-broker project. In that regard, the code can be considered dead by someone not aware of that project.

My idea is to split the oauth package into multiple modules, still part of the same package. This will add some level of namespacing and will split the source code into maintainable parts. Some aspects (basic authentication ones) may be split into separate packages.

To get a communication started, here is an initial unpolished API idea:

module.exports = {
  token: {
    parse: () => ... // function that parses a token entity (object/class?) from a string?/request?/header?
    request: () => ... // function that contacts UAA and requests a token
  },
  bearer: {
    authenticate: () => ..., // function that verifies the validity of an oauth token (authentication)
    authentication: () => ... // an express middleware that performs authentication
    authorize: () => ... // function that verifies the inclusion of required scopes in token
    authorization: () => .... // an express middleware that performs authorization
  }
};

I kind of also would like to separate the functions based on whether they operate on http entities (i.e. express related) or are raw functions working on strings / tokens.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/152329490

The labels on this github issue will be updated when the story is started.

[hsiliev]

Can we instead split this module into 2 (or 3) smaller ones?

[ghost]

Well, most of the stuff is indeed oauth related, so it makes sense to stay in this package, though I would like to split them up into different modules.

The decodeBasicToken should go somewhere else, I believe. Maybe a basic package, where other basic related functions can be added in the future.

The basicStrategy function is tricky. It escalates a Basic authorization call onto an OAuth one, by using the credentials to acquire an OAuth token. So it's neither pure basic, nor oauth. I would either leave it in oauth (since oauth is a higher-order package) or move it into a third (difficult to name) package.
I would also question the huge implementation of the function. It seems to include the validator middleware logic inside it. We could probably simplify this function by having it only convert the Authorization: Basic header into a Authorization: Bearer one and leave validation to a subsequent middleware in the application wiring.
Furthermore, it turns out that this logic is duplicated into the hystrix package. There are plenty of packages that can be optimized to use the middleware functions, instead of doing manual calls.

[ghost]

Also, I don't like how the validate and authorize functions have the same level of abstraction but differ in behavor. The former uses a callback mechanism to report problems, whereas the latter uses throw. The former does not even use the async version of jwt.verify to explain the usage of callbacks.

[hsiliev]

@momchil-sap Do we have more work here or can we close the issue?

[martin-d-aleksandrov]

Another point here: the cache() method accepts url that can be both cf api and uaa server, while the basicStrategy() one accepts only cf api url and would fail in case uaa url is passed. The naming of parameters in both methods is inadequate and misleading.

Another point for improvement here is to refactor the package in way that it accepts only one type of url. Preferably the uaa url, as the target uaa server in some cases could be not cloud foundry one.

[ghost]

@martin-d-aleksandrov , makes perfect sense.

@hsiliev , there is a lot more work here. I expect this to be an ongoing issue, until we manage to sort all small problems out. (i.e. expect multiple pull request references to this issue)