AngularJS - REST + Authentication service

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;

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 sho

StackExchange Code Review Q#103733, answer score: 17