Authentication system - AngularJS & Ionic Framework

Question

I am building a small mobile app using ionic framework and angularJS. It will consume a Restful Web Service that requires authentication (User token). This means I need to keep my users token by myself. Could anyone tell me if there is room for improvement in my code? Specifically regarding to my "Session" service which I feel kind of useless. I based my development in this article.

.run(function ($rootScope, EVENTS, AuthService) {
  $rootScope.$on('$stateChangeStart', function (event, next) {
    if (next.requiresAuth === true && !AuthService.isAuthenticated()) {
      event.preventDefault();
      $rootScope.$broadcast(EVENTS.notAuthenticated);
      console.log('You can not access here without login in.');
    }

  });
})
.constant('apiAddress', 'http://api.test.com')
.constant('EVENTS', {
  loginSuccess: 'auth-login-success',
  loginFailed: 'auth-login-failed',
  logoutSuccess: 'auth-logout-success',
  sessionTimeout: 'auth-session-timeout',
  notAuthenticated: 'auth-not-authenticated',
  signupSuccess: 'auth-signup-success'
})
.service('Session', function() {
  this.create = function(user) {
    this.user = user;
  };
  this.destroy = function() {
    this.user = null;
  }
  return this;
})
.factory('AuthService', function($http, $rootScope, $localstorage, Session, apiAddress, EVENTS) {
  var authService = {};
  authService.login = function(credentials){
    return $http.post(apiAddress + '/users/', credentials).then(function(res) {
      Session.create(res.data);
      $localstorage.setObject('user', res.data);
      return res.data;
    });
  };
  authService.signup = function(signForm){
    return $http.post(apiAddress + '/users/', signForm).then(function(res) {
      $rootScope.$broadcast(EVENTS.signupSuccess);
      return res.data;
    });
  };
  authService.logout = function(){
    Session.destroy();
    $localstorage.destroy('user');
    $rootScope.$broadcast(EVENTS.logoutSuccess);
  };
  authService.isAuthenticated = function() {
    return !!Session.user;
  };

  return authService;
})
controller('appCtrl', function($scope, $rootScope, $state, AuthService, EVENTS, $localstorage, Session) {
  $scope.currentUser = null;
  $scope.isAuthorized = AuthService.isAuthorized;
  $scope.setCurrentUser = function (user) {
    $scope.currentUser = user;
  };
  function logout() {
    AuthService.logout();
  }
  function redirectToLogin() {
    $state.go('login');
  }

  if ($localstorage.isSet('user') && Session.user == null) {
    Session.create($localstorage.getObject('user'));
    $scope.setCurrentUser($localstorage.getObject('user'))
    $state.go('cobranzas.clientes.index');
  }
  // We listen to auth related broadcasts.
  $scope.$on(EVENTS.notAuthenticated, logout);
  $scope.$on(EVENTS.sessionTimeout, logout);
  $scope.$on(EVENTS.signupSuccess, logout);
  $scope.$on(EVENTS.logoutSuccess, redirectToLogin);
})
.controller('loginCtrl', function($scope, $rootScope, $state, EVENTS, AuthService) {
  $scope.credentials = {
    username: '',
    password: ''
  };
  $scope.login = function (credentials) {
    AuthService.login(credentials).then(function (user) {
      $rootScope.$broadcast(EVENTS.loginSuccess);
      $scope.setCurrentUser(user);
      $state.go('index');
    }, function () {
      $rootScope.$broadcast(EVENTS.loginFailed);
    });
  };
})
.controller('signupCtrl', function($scope, $rootScope, $state, EVENTS, AuthService) {
  $scope.signForm = {
    FirstName: '',
    LastName: '',
    Password: '',
    Country: '',
    Phone: '',
    Gender: '',
    Position: ''
  };
  $scope.signup = function (signForm) {
    AuthService.signup(signForm).then(function (data) {
      $state.go('login');
    }, function (data) {
      console.log("Failed");
    });
  };
})

I don't feel right storing user in scope and in the service. Also I feel weird verifying localstorage in app controller. (App controller is a controller placed high at DOM so it's inherited by the others).

Answer 1

First thing that strikes me is the inconsistency of naming. Why using different styles 'apiAddress', 'EVENTS' for constants?

Your .service does not need to return this, see here.

I would use something like signData instead of signForm as Form has another meaning.

As a personal preference I like to write my .factory as

.factory('AuthService', function (...) {

   // any initialization and private method go here
     ...

   return {

      // public methods
      login: function (...) {...},
         ...

   }
});

This keeps my code DRYer and I don't need to worry misprinting authService or forgetting to return it (your factory does need a return!)

You don't handle errors (but you knew this already ;-)

Your .controller has way too many dependencies and responsibilities. The best practice is to keep your controllers "thin" with the sole responsibility to glue your data with its scope. Changing routes (states) looks like one job too much. Say you want to keep another sing-up form elsewhere with its own controller - will you really want to copy over all the logic?

This is good for prototyping but is generally to be avoided in production:

console.log("Failed");

Also this way of using promise is not recommended - notice that your second (error) function will not catch any errors inside the first (success) function. A better way is:

AuthService.signup(signForm)
  .then(function (data) {

     // handle your success here

  })
  .error(function (error) {

     // handle your error here

  });

Now you will catch all errors.

Finally, I find your Session too small and AuthService too big with too many dependencies. It plays a role of a "Manager" that has many responsibilities. You can't always avoid it but you want to keep Manager's jobs at high level and as simple as possible. Imagine a stupid manager that knows nothing other than how to give basic commands :)

.factory('AuthService', function (...) {
   return {
      login: function (credentials) {
         Server
           .login(credentials)
           .then(function (user) {

              // we return the result promise that can be picked by next function
              return UserService.create(user);
           })
           .error(function (error) {
             ...
           })
      }
   }
})