AngularJS - REST + Authentication service

Question

I have a REST web service that uses OAuth 2 for authenticating and authorizing requests.

I have an endpoint, that when receiving the correct credentials, responds with an access token that will be used in consequent requests.

This avoids persisting a session and using cookies, which is what we wanted to accomplish when we started designing our web service.

Along with the access token a request token is also received from the response. This refresh token is used for getting a new access token when the original one has expired.

With short timespan and token refreshing, an user can't authenticate as another user just by stealing the access token (theorically it can, but for a short period of time, i.e. until it's refreshed).

I have created the following provider for AngularJS:

var restProvider = function () {

     var _enabled = true;
     var _authUrl = '';
     var _clientId = '';
     var _sessionLife;

     // Returns whether the service is available or not
     var _supported = function () {
         return _enabled && _authUrl;
     }

     var _fetchTokens = function (http, deferred, url, credentials, storage) {
          credentials.client_id = _clientId;
          credentials.grant_type = 'password';

          http.post(url, credentials, {
              headers: {
                 'Content-Type': 'application/x-www-form-urlencoded'
              },
              transformRequest: function (data, headers) {
                 var str = [];
                 for(var p in data)
                    str.push(encodeURIComponent(p) + '=' + encodeURIComponent(data[p]));
                 return str.join('&');
              }
          }).then(function (successResponse) {
               storage.save('access_token', successResponse.data.access_token, {
                  expiration: successResponse.data.expires_in;
               });
               storage.save('refresh_token', successResponse.data.refresh_token, {
                  expiration: _sessionLife;
               });
               storage.save('token_type', successResponse.data.token_type);

               deferred.resolve(successResponse);
          }, function (errorResponse) {
               deferred.reject(errorResponse);
          });
     }

     return {
         //The following methods are for configuration only
         //If no arguments are provided, returns the property's value
         //If arguments are provided, returns "this" for chaining multiple methods
         //val: Boolean
         enabled: function (val) {
             if(val === undefined || val === null) return _enabled;
             _enabled = val;
             return this;
         },

         //endpoint: String
         authorizationEndpoint: function (endpoint) {
             if(!endpoint) return _authUrl;
             _authUrl = endpoint;
             return this;
         },

         //clientId: String
         client: function (clientId) {
            if(!clientId) return _clientId;
            _clientId = clientId;
            return this;
         },

         //seconds: Number
         sessionLife: function (seconds) {
            if(!seconds) return _sessionLife;
            _sessionLife = seconds;
            return this;
         },

         //"storage" and "logger" are injected services of mine
         $get: ['$http', '$q', 'storage', 'logger', function ($http, $q, storage, logger) {
              return {
                   signIn: function (username, password) {
                       var deferred = $q.defer();
                       if(username && password && _supported())
                           _fetchTokens($http, deferred, _authUrl, { username: username, password: password }, storage);
                       else
                           deferred.reject();
                       return deferred.promise;
                   }
              }
         }
     }
}

As you can see it's a very long code and it's even incomplete, this is only the authenticating function.

_fetchTokens makes a POST request to my web service's OAuth endpoint and retrieves the tokens form the response. It then proceeds to persist them (For example, localStorage) so they can be used for later requests and continue to live on even if the application is closed.

signIn is the public function that the client will see from outside. It simply creates the deferred object and calls _fetchTokens.

My problem is that consequent requests should send the access token (on the Authorization header). If a request is sent with an expired token, a 401 Unauthorized is returned, so this service must proceed to try to refresh the token. After successfully refreshing the token, it shall retry the original request again but with the new access token.

This means that a simple line of code from the client:

service.get('api.example.com/products');

Should do all this behind the scenes.

With this approach I think there's going to be a lot of replicated code, so I was looking for help on how to better structure my code. Not only for better quality but also for better readability.

I suppose such a service is not that uncommon so I hope that many people has experienced this type of situations.

Answer 1

Avoiding the fact this code appears to be off-topic (because you mention it's incomplete), I still think it would prompt an interesting review.

I'm so pretty

Let's first address some superficial concerns:

You have an inconsistent use of semi-colons. Using semi-colons now is more or less a stylistic choice due to ASI, however if you're going to use semi-colons, you should be consistent in your application and use them in all relevant places, or nowhere at all. This means adding a semi colon after function expressions (var foo = function() {};) and after return statements.

Next up, your naming convention. Whilst I do recognise the _leadingUnderscore convention to mean "private variables", in JavaScript we generally just use camelCase for everything. There is no concept of private variable in JavaScript, and in Angular's source, private variables are denoted by $$variableName. Ultimately again a stylistic choice, but I would prefer to see you just use fetchTokens rather than _fetchTokens. In my opinion, this is more readable as there is less 'noise' for the eye to process.

Now we have your usage of Angular services. You should probably always denote Angular services with their fully qualified name, that is, with the $leadingDollarSign. So http should be $http. This is because it makes it easier for me to immediately recognise "aha, this will definitely be Angular's $http service" instead of there being some ambiguity.

Access tokens

I am... unsure of how you use your access token. You appear to store it in localStorage. This is great for de/re-hydration of a session for a client, however, I don't see you actually appending your access token to any requests.. in addition, your question asks how you would use your access token. Your answer lies in the use of a $http.defaults authorization token, although this does fall apart when you use multiple APIs from different domains. If you are doing so, you will have to use a http interceptor (which will give you more control). If you're not doing that, then the following line should do what you want:

$http.defaults.headers.common.Authorization = 'Bearer ' + accessToken;

Be wary that this can be changed at any point from any other point in the application. If this makes you uneasy, again, use an interceptor.

Dirty little secretses

This line:

str.push(encodeURIComponent(p) + '=' + encodeURIComponent(data[p]));

Please, no. This is really bad. The data you post in this call will be transformed into query strings. I'm not sure why you did this originally, because this is a security hazard. By putting the credentials into the query string, the username and password will be visible to anyone who can actually see the URL - which is the client, anyone standing over your shoulder, anyone who looks through your browser history, anyone snooping on his connection (even if he is using https), and your server logs. You do not want this information in any of those locations.

Instead, keep the username and password credentials inside of the http body, where they belong. There they will be encrypted using https (you are using https, right?). In any case, to achieve a similar functionality, providing you're using a more recent version of Angular (I believe it is 1.4+), you can use $httpParamSerializer which will avoid you having to do that nasty for loop and encodeURIComponent you have.

Seriously though, don't put username and passwords in the url. JWT access tokens are okay because those expire (and you can implement other measures with them), but you can't do that with a username and password.

Manual annotation

If you're using a build step (why aren't you?) you can use ng-annotate, which wil prevent you from having to sepcify your dependencies 2 times. Specifying your dependencies 2 times is a serious case of code duplication that can amount to large headaches for maintainability (trust me, I know, in my current company we AREN'T using a build process and we have to specify our dependencies in 4 places).

Function expressions vs Function declaration.

A function expression looks like this:

var foo = function() {}

A function declaration looks like this:

function foo() {}

In my opinion you should always use the second approach where possible - this will give you a name on your function and make your stacktraces significantly easier to read. Alternatively, you can use a hybrid approach..

var foo = function foo() {}

This gives you the best of both world. The main reason to have the function name is simply that then you have a better stack trace (function names are displayed in stack traces). If you didn't have this here, you would simply see "anonymous function" - which helps no-one and you will have to step into the source code to actually see the problem.

_supported

This function doesn't make sense - either the service is supported, or it isn't. In any case, if the service isn't supported, then it should not be included in your application - put it in a separate module and don't include it in that case. Either way, this function will always return true in it's current implementation and so I feel that it is redundant. The same goes for _enabled and enabled(); neither of these make any sense. In addition, you probably aren't going to want to expose the power to switch authentication on and off as this would cause a mess in your app - anyone could call this function and modify the state to cause issues. It would be better if you keep this as immutable as possible and simply always have it enabled. It's up to your server whether or not to respond to the Authentication header.

_sessionLife

This doesn't make any sense either; the server should determine how long the session should be and should inform the client of when the session has ended via a 403 response on an authenticated HTTP request. Having the client know about how long a session should be - unless you have fixed-length sessions - doesn't make any sense from a separation of concerns POV.

Session tokens are opaque objects

Session tokens should be an opaque object. As I mentioned in _sessionLife, the server should be determining the session life as session life is not a deterministic thing - the session life may last 15 minutes normally, but the whole idea of using tokens is that you're stateless on the client, but also you can revoke sessions at any time. Having a session length here doesn't really make sense - just have the client respond to when a session has expired from an Unauthenticated HTTP response.

Promise chaining

You don't need to pass a promise into each function - you can instead chain promises. That's hard to explain but essentially the returned result from one then function will be passed on to the next then in the chain - you'll have to see my code to see how that works.

Putting it all together

After all of these points, here is your amended code using my guidance. Bear in mind that what I have said is not a definitive guide on anything and I am not an authority on AngularJS; everything here (except for your query string thing) is subjective. Note that I have done some rather primitive dead-code elimination on your code (such as removing the refresh token and token type in localStorage). From what I can see, neither of those are used. If I have removed anything important, please tell me what it is used for and I'll add it back, however this is why you should always post the full context ;-)

I've also removed your setters/getters because I don't feel that they are very useful, frankly. Inject those settings using Angular's dependency injection. We've already established that sessionLife and enabled make no sense; client and authorizationEndpoint are not exactly configurable values, in my opinion. Make them constants in your angular module (if you use them at all).

You'll notice a few // @ngInject. This is a signal to ngAnnotate to annotate that function. If you're not using ngAnnotate, you'll need to replace these with an $inject property on that function instance (or with the standard array notation).

function restProvider() {
  var authUrl = '';
  var clientId = '';

  // You have no need for two separate functions for signing in.
  // @ngInject
  function signIn($http, url, username, password) {
    var credentials = {
      username: username, 
      password: password, 
      client_id: clientId,
      grant_type: 'password'
    };

    return $http.post(url, credentials)
      .then(function(response) {
        var payload = response.data;
        var accessToken = payload.access_token;
        // When you refresh another token this will just be overwritten.
        $http.defaults.headers.common.Authorization = 'Bearer ' + accessToken;
      });
  }

  return {
    // @ngInject
    $get: function($http) {
      return {
        // This partially applies signIn with $http and authUrl. The next invocation will only take
        // username and password as parameters. "this" is set to null.
        signIn: signIn.bind(null, $http, authUrl)
      };
    }
  };
}

Answer 2

I think these are different layers of responsibility.

The service.get('api.example.com/products'); is called in the controller to the corresponding view. What I suggest is the following:

You have a common folder, where all your shared services and functions are placed.

Inside this, you create following services:

• api.js // to handle all your CRUD calls to the backend
• authentication-service.js // with login(), logout(), isAuthenticated(), register(), getToken() etc.
• authentication-session.js // for storing, getting, and deleting the tokens

Therefore, you can:

• change the URL for the APIs without touching any other code.
• switch even between cookie, browserStorage or other token-storage solutions without interfering other parts of your app

In the authentication-service.js, you also have a function like this to send your token on every request to the backend:

  authService.setAuthenticationHeader = function() {
    $http.defaults.headers.common['x-access-token'] = AuthenticationSession.token;
  };

Furthermore, since you are using AngularJS, play around with interceptors:

angular
  .module( 'interceptors', [])
  .factory('HttpInterceptors', HttpInterceptors);

HttpInterceptors.$inject = ['$q', '$injector'];

function HttpInterceptors($q, $injector) {
  return {
    // On request success
    request: function (config) {
      // console.log(config); // Contains the data about the request before it is sent.

      // Return the config or wrap it in a promise if blank.
      return config || $q.when(config);
    },

    // On request failure
    requestError: function (rejection) {
      //console.log('rejection', rejection); // Contains the data about the error on the request.

      // Return the promise rejection.
      return $q.reject(rejection);
    },

    // On response success
    response: function (response) {
      // console.log(response); // Contains the data from the response.

      // Return the response or promise.
      return response || $q.when(response);
    },

    // On response failture
    responseError: function (rejection) {
      if(rejection.status === 401 || rejection.status === 400) {
        var state = $injector.get('$state');
        state.go('login');
      }

      // Return the promise rejection.
      return $q.reject(rejection);
    }
  };
}

So you can handle every return code from every HTTP call throughout your app from one place.

Folder structure:

• /app/common/api/api.js
• /app/common/authentication/authentication-service.js
• /app/common/authentication/authentication-session.js
• /app/common/interceptors/http-interceptor.js

You can even think of creating a product-service.js as your model, and inside there, you call the api.js to get new products. Then, inside the controller, you just say products.getLatest() or something.

I have an example repository which I created to follow some rules and principles. You can browse through, I think it's mainly the same procedure every time.