changeset 185:c10369cb53a6

Use LocalStorage rather than cookies for basic-auth state Reviewed-by: jkang Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2017-September/024862.html
author Andrew Azores <aazores@redhat.com>
date Wed, 06 Sep 2017 10:17:25 -0400
parents 81d3963dd455
children 4da0690286ac
files package.json src/app/app.controller.spec.js src/app/app.module.js src/app/components/auth/basic-auth.service.js src/app/components/auth/basic-auth.service.spec.js src/app/components/auth/login/login.controller.js src/app/components/user-prefs/user-prefs.service.js src/app/components/user-prefs/user-prefs.service.spec.js src/app/shared/services/local-storage.service.js src/app/shared/services/local-storage.service.spec.js
diffstat 10 files changed, 279 insertions(+), 76 deletions(-) [+]
line wrap: on
line diff
--- a/package.json	Fri Sep 08 08:31:47 2017 -0400
+++ b/package.json	Wed Sep 06 10:17:25 2017 -0400
@@ -11,7 +11,6 @@
   "license": "",
   "devDependencies": {
     "@uirouter/angularjs": "^1.0.0",
-    "angular-cookies": "1.5.*",
     "angular-mocks": "1.5.*",
     "angular-patternfly": "^4.4.1",
     "angular-translate": "^2.15.2",
--- a/src/app/app.controller.spec.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/app.controller.spec.js	Wed Sep 06 10:17:25 2017 -0400
@@ -31,12 +31,14 @@
     'ngInject';
     $provide.value('$transitions', { onBefore: angular.noop });
 
-    let cookies = {
-      put: sinon.spy(),
-      get: sinon.stub().returns('fake-username'),
-      remove: sinon.spy()
+    let localStorage = {
+      getItem: sinon.stub(),
+      hasItem: sinon.stub(),
+      removeItem: sinon.spy(),
+      setItem: sinon.spy(),
+      clear: sinon.spy()
     };
-    $provide.value('$cookies', cookies);
+    $provide.value('localStorage', localStorage);
   }));
 
   beforeEach(angular.mock.module('AppController'));
--- a/src/app/app.module.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/app.module.js	Wed Sep 06 10:17:25 2017 -0400
@@ -27,7 +27,6 @@
 
 import 'angular-patternfly';
 import '@uirouter/angularjs';
-import 'angular-cookies';
 import angularTranslate from 'angular-translate';
 import 'angular-translate-interpolation-messageformat';
 import 'oclazyload';
@@ -54,7 +53,6 @@
   .module('appModule', [
     'ui.router',
     'ui.bootstrap',
-    'ngCookies',
     angularTranslate,
     configModule,
     authModule,
--- a/src/app/components/auth/basic-auth.service.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/components/auth/basic-auth.service.js	Wed Sep 06 10:17:25 2017 -0400
@@ -27,13 +27,15 @@
 
 import * as url from 'url';
 
+const SESSION_EXPIRY_MINUTES = 15;
+
 export default class BasicAuthService {
 
-  constructor ($q, $state, $cookies) {
+  constructor ($q, $state, localStorage) {
     'ngInject';
     this.q = $q;
     this.$state = $state;
-    this.cookies = $cookies;
+    this._localStorage = localStorage;
 
     this._pass = null;
   }
@@ -43,17 +45,17 @@
   }
 
   status () {
-    return angular.isDefined(this.cookies.get('session'));
+    return this._sessionIsValid();
   }
 
   login (user, pass, success = angular.noop) {
     this._pass = pass;
     if (this._rememberUser) {
-      this.cookies.put('username', user);
+      this._localStorage.setItem('username', user);
     }
 
+    this._localStorage.setItem('loggedInUser', user);
     this._refreshSession();
-    this.cookies.put('loggedInUser', user);
 
     this._rootScope.$broadcast('userLoginChanged');
     success();
@@ -67,8 +69,8 @@
     this._pass = null;
     this.$state.go('login');
 
-    this.cookies.remove('session');
-    this.cookies.remove('loggedInUser');
+    this._localStorage.removeItem('session');
+    this._localStorage.removeItem('loggedInUser');
 
     this._rootScope.$broadcast('userLoginChanged');
     callback();
@@ -77,19 +79,28 @@
   _refreshSession () {
     let now = new Date();
     let expiry = new Date(now);
-    expiry.setMinutes(now.getMinutes() + 15);
-    this.cookies.put('session', true, { expires: expiry });
+    expiry.setMinutes(now.getMinutes() + SESSION_EXPIRY_MINUTES);
+    this._localStorage.setItem('session', expiry);
+  }
+
+  _sessionIsValid () {
+    if (!this._localStorage.hasItem('session')) {
+      return false;
+    }
+    let session = new Date(this._localStorage.getItem('session'));
+    let now = new Date();
+
+    return session.getTime() >= now.getTime();
   }
 
   refresh () {
     let defer = this.q.defer();
-    let session = this.cookies.get('session');
-    if (session) {
+    if (this._sessionIsValid()) {
       this._refreshSession();
       defer.resolve();
     } else {
-      this.cookies.remove('session');
-      this.cookies.remove('loggedInUser');
+      this._localStorage.removeItem('session');
+      this._localStorage.removeItem('loggedInUser');
       defer.reject();
     }
     return defer.promise;
@@ -100,11 +111,11 @@
   }
 
   get username () {
-    return this.cookies.get('loggedInUser');
+    return this._localStorage.getItem('loggedInUser');
   }
 
   get rememberedUsername () {
-    return this.cookies.get('username');
+    return this._localStorage.getItem('username');
   }
 
   getCommandChannelUrl (baseUrl) {
@@ -124,7 +135,7 @@
   rememberUser (remember) {
     this._rememberUser = remember;
     if (!remember) {
-      this.cookies.remove('username');
+      this._localStorage.removeItem('username');
     }
   }
 
--- a/src/app/components/auth/basic-auth.service.spec.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/components/auth/basic-auth.service.spec.js	Wed Sep 06 10:17:25 2017 -0400
@@ -30,7 +30,14 @@
 import BasicAuthService from './basic-auth.service.js';
 
 describe('BasicAuthService', () => {
-  let basicAuthService, q, qPromise, state, cookies, rootScope;
+
+  function future () {
+    let now = new Date();
+    now.setMinutes(now.getMinutes() + 30);
+    return now;
+  }
+
+  let basicAuthService, q, qPromise, state, localStorage, rootScope;
   beforeEach(() => {
     qPromise = sinon.stub().yields();
     q = {
@@ -46,13 +53,15 @@
       go: sinon.spy(),
       target: sinon.stub().returns('stateTarget')
     };
-    cookies = {
-      put: sinon.spy(),
-      get: sinon.stub(),
-      remove: sinon.spy()
+    localStorage = {
+      getItem: sinon.stub(),
+      hasItem: sinon.stub(),
+      removeItem: sinon.spy(),
+      setItem: sinon.spy(),
+      clear: sinon.spy()
     };
     rootScope = { $broadcast: sinon.spy() };
-    basicAuthService = new BasicAuthService(q, state, cookies);
+    basicAuthService = new BasicAuthService(q, state, localStorage);
     basicAuthService.rootScope = rootScope;
   });
 
@@ -63,8 +72,10 @@
   describe('#login()', () => {
     it('should set logged in status on successful login', done => {
       basicAuthService.login('client', 'client-pwd', () => {
-        cookies.put.should.be.calledWith('session', true, sinon.match.object);
-        cookies.get.withArgs('session').returns(true);
+        localStorage.setItem.should.be.calledWith('loggedInUser', 'client');
+        localStorage.setItem.should.be.calledWith('session', sinon.match.date);
+        localStorage.getItem.withArgs('session').returns(future());
+        localStorage.hasItem.withArgs('session').returns(true);
         basicAuthService.status().should.equal(true);
         done();
       });
@@ -73,8 +84,8 @@
     it('should set username on successful login', done => {
       should(basicAuthService.username).be.undefined();
       basicAuthService.login('client', 'client-pwd', () => {
-        cookies.put.should.be.calledWith('loggedInUser', 'client');
-        cookies.get.withArgs('loggedInUser').returns('client');
+        localStorage.setItem.should.be.calledWith('loggedInUser', 'client');
+        localStorage.getItem.withArgs('loggedInUser').returns('client');
         basicAuthService.username.should.equal('client');
         done();
       });
@@ -82,8 +93,8 @@
 
     it('should not store username in cookies by default', done => {
       basicAuthService.login('client', 'client-pwd', () => {
-        cookies.put.should.be.called();
-        cookies.put.should.not.be.calledWith('username');
+        localStorage.setItem.should.be.called();
+        localStorage.setItem.should.not.be.calledWith('username');
         done();
       });
     });
@@ -91,10 +102,10 @@
     it('should store username in cookies when set', done => {
       basicAuthService.rememberUser(true);
       basicAuthService.login('client', 'client-pwd', () => {
-        cookies.put.should.be.calledThrice();
-        cookies.put.should.be.calledWith('loggedInUser', 'client');
-        cookies.put.should.be.calledWith('username', 'client');
-        cookies.put.should.be.calledWith('session', true, sinon.match.object);
+        localStorage.setItem.should.be.calledThrice();
+        localStorage.setItem.should.be.calledWith('loggedInUser', 'client');
+        localStorage.setItem.should.be.calledWith('username', 'client');
+        localStorage.setItem.should.be.calledWith('session', sinon.match.date);
         done();
       });
     });
@@ -121,12 +132,14 @@
   describe('#logout()', () => {
     it('should set logged out status', done => {
       basicAuthService.login('client', 'client-pwd');
-      cookies.put.should.be.calledWith('session', true, sinon.match.object);
-      cookies.get.withArgs('session').returns(true);
+      localStorage.setItem.should.be.calledWith('session', sinon.match.date);
+      localStorage.getItem.withArgs('session').returns(future());
+      localStorage.hasItem.withArgs('session').returns(true);
       basicAuthService.status().should.equal(true);
       basicAuthService.logout(() => {
-        cookies.remove.should.be.calledWith('session');
-        cookies.get.withArgs('session').returns(undefined);
+        localStorage.removeItem.should.be.calledWith('session');
+        localStorage.getItem.withArgs('session').returns(null);
+        localStorage.hasItem.withArgs('session').returns(false);
         basicAuthService.status().should.equal(false);
         done();
       });
@@ -167,8 +180,9 @@
 
     it('should call success handler if logged in', done => {
       basicAuthService.login('foo', 'bar', () => {
-        cookies.put.should.be.calledWith('session', true, sinon.match.object);
-        cookies.get.withArgs('session').returns(true);
+        localStorage.setItem.should.be.calledWith('session', sinon.match.date);
+        localStorage.getItem.withArgs('session').returns(future());
+        localStorage.hasItem.withArgs('session').returns(true);
         basicAuthService.refresh().then(done, angular.noop);
       });
     });
@@ -176,8 +190,9 @@
     it('should call error handler if logged out', done => {
       qPromise.callsArg(1);
       basicAuthService.logout();
-      cookies.remove.should.be.calledWith('session');
-      cookies.get.withArgs('session').returns(false);
+      localStorage.removeItem.should.be.calledWith('session');
+      localStorage.getItem.withArgs('session').returns(null);
+      localStorage.hasItem.withArgs('session').returns(false);
       basicAuthService.refresh().then(angular.noop, done);
     });
   });
@@ -185,7 +200,7 @@
   describe('#get authHeader()', () => {
     it('should return base64-encoded credentials', done => {
       basicAuthService.login('foo', 'bar', () => {
-        cookies.get.withArgs('loggedInUser').returns('foo');
+        localStorage.getItem.withArgs('loggedInUser').returns('foo');
         basicAuthService.authHeader.should.equal('Basic ' + btoa('foo:bar'));
         done();
       });
@@ -199,7 +214,7 @@
 
     it('should return logged in user', done => {
       basicAuthService.login('foo', 'bar', () => {
-        cookies.get.withArgs('loggedInUser').returns('foo');
+        localStorage.getItem.withArgs('loggedInUser').returns('foo');
         basicAuthService.username.should.equal('foo');
         done();
       });
@@ -208,10 +223,10 @@
 
   describe('#get rememberedUsername()', () => {
     it('should return username stored in cookie', () => {
-      cookies.get.returns('fakeUser');
-      cookies.get.should.not.be.called();
+      localStorage.getItem.returns('fakeUser');
+      localStorage.getItem.should.not.be.called();
       basicAuthService.rememberedUsername.should.equal('fakeUser');
-      cookies.get.should.be.calledOnce();
+      localStorage.getItem.should.be.calledOnce();
     });
   });
 
@@ -223,7 +238,7 @@
 
     it('should only add basic auth username when only username provided', done => {
       basicAuthService.login('foo', null, () => {
-        cookies.get.withArgs('loggedInUser').returns('foo');
+        localStorage.getItem.withArgs('loggedInUser').returns('foo');
         basicAuthService.getCommandChannelUrl('http://example.com/').should.equal('http://foo@example.com/');
         done();
       });
@@ -231,7 +246,7 @@
 
     it('should add basic auth username and password when provided', done => {
       basicAuthService.login('foo', 'bar', () => {
-        cookies.get.withArgs('loggedInUser').returns('foo');
+        localStorage.getItem.withArgs('loggedInUser').returns('foo');
         basicAuthService.getCommandChannelUrl('http://example.com/').should.equal('http://foo:bar@example.com/');
         done();
       });
@@ -241,9 +256,9 @@
   describe('rememberUser', () => {
     it('should remove username stored in cookie when called with "false"', () => {
       basicAuthService.rememberUser(true);
-      cookies.remove.should.not.be.called();
+      localStorage.removeItem.should.not.be.called();
       basicAuthService.rememberUser(false);
-      cookies.remove.should.be.calledWith('username');
+      localStorage.removeItem.should.be.calledWith('username');
     });
   });
 });
--- a/src/app/components/auth/login/login.controller.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/components/auth/login/login.controller.js	Wed Sep 06 10:17:25 2017 -0400
@@ -38,7 +38,7 @@
       return;
     }
 
-    this.rememberUser = angular.isDefined(authService.rememberedUsername);
+    this.rememberUser = authService.rememberedUsername != null;
     if (this.rememberUser) {
       this.username = authService.rememberedUsername;
     }
--- a/src/app/components/user-prefs/user-prefs.service.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/components/user-prefs/user-prefs.service.js	Wed Sep 06 10:17:25 2017 -0400
@@ -25,33 +25,32 @@
  * exception statement from your version.
  */
 
+import services from 'shared/services/services.module.js';
 import * as url from 'url';
 
 class UserPreferencesService {
-  constructor ($cookies) {
+  constructor (localStorage) {
     'ngInject';
-    this._cookies = $cookies;
+    this._storage = localStorage;
   }
 
   set tlsEnabled (tlsEnabled) {
-    this._cookies.put('tlsEnabled', JSON.parse(tlsEnabled));
+    this._storage.setItem('tlsEnabled', JSON.parse(tlsEnabled));
   }
 
   get tlsEnabled () {
-    let raw = this._cookies.get('tlsEnabled');
     // can't use gatewayUrl value here due to circular reference, but process.env
     // is not available in test suite
     /* istanbul ignore next */
-    if (!angular.isDefined(raw)) {
+    if (!this._storage.hasItem('tlsEnabled')) {
       let protocol = url.parse(process.env.GATEWAY_URL).protocol;
       this.tlsEnabled = protocol === 'https:';
-      raw = this._cookies.get('tlsEnabled');
     }
-    return JSON.parse(raw);
+    return JSON.parse(this._storage.getItem('tlsEnabled'));
   }
 }
 
 export default angular
-  .module('userPrefs.service', ['ngCookies'])
+  .module('userPrefs.service', [services])
   .service('userPrefsService', UserPreferencesService)
   .name;
--- a/src/app/components/user-prefs/user-prefs.service.spec.js	Fri Sep 08 08:31:47 2017 -0400
+++ b/src/app/components/user-prefs/user-prefs.service.spec.js	Wed Sep 06 10:17:25 2017 -0400
@@ -25,21 +25,24 @@
  * exception statement from your version.
  */
 
+import services from 'shared/services/services.module.js';
 import service from './user-prefs.service.js';
 
 describe('userPrefsService', () => {
 
-  let svc, cookies;
+  let svc, storage;
   beforeEach(() => {
-    cookies = {
-      get: sinon.stub().returns(),
-      put: sinon.spy()
+    storage = {
+      getItem: sinon.stub(),
+      setItem: sinon.spy(),
+      hasItem: sinon.stub()
     };
-    angular.mock.module(service);
+    angular.mock.module(services);
     angular.mock.module($provide => {
       'ngInject';
-      $provide.value('$cookies', cookies);
+      $provide.value('localStorage', storage);
     });
+    angular.mock.module(service);
     angular.mock.inject(userPrefsService => {
       'ngInject';
       svc = userPrefsService;
@@ -50,14 +53,14 @@
     should.exist(svc);
   });
 
-  it('should store tlsEnabled preference in cookies', () => {
-    cookies.put.should.not.be.called();
+  it('should store tlsEnabled preference in storage', () => {
+    storage.setItem.should.not.be.called();
     svc.tlsEnabled = 'false';
-    cookies.put.should.be.calledWith('tlsEnabled', false);
+    storage.setItem.should.be.calledWith('tlsEnabled', false);
   });
 
   it('should return stored cookie value when present', () => {
-    cookies.get.returns(true);
+    storage.getItem.returns(true);
     svc.tlsEnabled.should.equal(true);
   });
 
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/src/app/shared/services/local-storage.service.js	Wed Sep 06 10:17:25 2017 -0400
@@ -0,0 +1,62 @@
+/**
+ * Copyright 2012-2017 Red Hat, Inc.
+ *
+ * Thermostat is distributed under the GNU General Public License,
+ * version 2 or any later version (with a special exception described
+ * below, commonly known as the "Classpath Exception").
+ *
+ * A copy of GNU General Public License (GPL) is included in this
+ * distribution, in the file COPYING.
+ *
+ * Linking Thermostat code with other modules is making a combined work
+ * based on Thermostat.  Thus, the terms and conditions of the GPL
+ * cover the whole combination.
+ *
+ * As a special exception, the copyright holders of Thermostat give you
+ * permission to link this code with independent modules to produce an
+ * executable, regardless of the license terms of these independent
+ * modules, and to copy and distribute the resulting executable under
+ * terms of your choice, provided that you also meet, for each linked
+ * independent module, the terms and conditions of the license of that
+ * module.  An independent module is a module which is not derived from
+ * or based on Thermostat code.  If you modify Thermostat, you may
+ * extend this exception to your version of the software, but you are
+ * not obligated to do so.  If you do not wish to do so, delete this
+ * exception statement from your version.
+ */
+
+import servicesModule from 'shared/services/services.module.js';
+
+class LocalStorageService {
+
+  constructor ($window) {
+    'ngInject';
+    this._storage = $window.localStorage;
+  }
+
+  setItem (key, value) {
+    this._storage.setItem(key, value);
+  }
+
+  getItem (key) {
+    return this._storage.getItem(key);
+  }
+
+  removeItem (key) {
+    this._storage.removeItem(key);
+  }
+
+  hasItem (key) {
+    let item = this.getItem(key);
+    return angular.isDefined(item) && item != null;
+  }
+
+  clear () {
+    this._storage.clear();
+  }
+
+}
+
+angular
+  .module(servicesModule)
+  .service('localStorage', LocalStorageService);
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/src/app/shared/services/local-storage.service.spec.js	Wed Sep 06 10:17:25 2017 -0400
@@ -0,0 +1,114 @@
+/**
+ * Copyright 2012-2017 Red Hat, Inc.
+ *
+ * Thermostat is distributed under the GNU General Public License,
+ * version 2 or any later version (with a special exception described
+ * below, commonly known as the "Classpath Exception").
+ *
+ * A copy of GNU General Public License (GPL) is included in this
+ * distribution, in the file COPYING.
+ *
+ * Linking Thermostat code with other modules is making a combined work
+ * based on Thermostat.  Thus, the terms and conditions of the GPL
+ * cover the whole combination.
+ *
+ * As a special exception, the copyright holders of Thermostat give you
+ * permission to link this code with independent modules to produce an
+ * executable, regardless of the license terms of these independent
+ * modules, and to copy and distribute the resulting executable under
+ * terms of your choice, provided that you also meet, for each linked
+ * independent module, the terms and conditions of the license of that
+ * module.  An independent module is a module which is not derived from
+ * or based on Thermostat code.  If you modify Thermostat, you may
+ * extend this exception to your version of the software, but you are
+ * not obligated to do so.  If you do not wish to do so, delete this
+ * exception statement from your version.
+ */
+
+import servicesModule from 'shared/services/services.module.js';
+
+describe('localStorage', () => {
+
+  let svc, mockStorage;
+  beforeEach(() => {
+    angular.mock.module($provide => {
+      'ngInject';
+      mockStorage = {
+        setItem: sinon.spy(),
+        getItem: sinon.stub(),
+        removeItem: sinon.spy(),
+        hasItem: sinon.stub(),
+        clear: sinon.spy()
+      };
+      let wdw = {
+        localStorage: mockStorage
+      };
+      $provide.value('$window', wdw);
+    });
+
+    angular.mock.module(servicesModule);
+
+    angular.mock.inject(localStorage => {
+      'ngInject';
+      svc = localStorage;
+    });
+  });
+
+  it('should exist', () => {
+    should.exist(svc);
+  });
+
+  describe('setItem', () => {
+    it('should delegate', () => {
+      mockStorage.setItem.should.not.be.called();
+      svc.setItem('foo', 'bar');
+      mockStorage.setItem.should.be.calledOnce();
+      mockStorage.setItem.should.be.calledWith('foo', 'bar');
+    });
+  });
+
+  describe('getItem', () => {
+    it('should delegate', () => {
+      mockStorage.getItem.should.not.be.called();
+      mockStorage.getItem.returns('getItemMock');
+      svc.getItem('foo').should.equal('getItemMock');
+      mockStorage.getItem.should.be.calledOnce();
+      mockStorage.getItem.should.be.calledWith('foo');
+    });
+  });
+
+  describe('removeItem', () => {
+    it('should delegate', () => {
+      mockStorage.removeItem.should.not.be.called();
+      svc.removeItem('foo');
+      mockStorage.removeItem.should.be.calledOnce();
+      mockStorage.removeItem.should.be.calledWith('foo');
+    });
+  });
+
+  describe('hasItem', () => {
+    it('should return false on undefined', () => {
+      mockStorage.getItem.withArgs('foo').returns(undefined);
+      svc.hasItem('foo').should.equal(false);
+    });
+
+    it('should return false on null', () => {
+      mockStorage.getItem.withArgs('foo').returns(null);
+      svc.hasItem('foo').should.equal(false);
+    });
+
+    it('should return true on other', () => {
+      mockStorage.getItem.withArgs('foo').returns(5);
+      svc.hasItem('foo').should.equal(true);
+    });
+  });
+
+  describe('clear', () => {
+    it('should delegate', () => {
+      mockStorage.clear.should.not.be.called();
+      svc.clear();
+      mockStorage.clear.should.be.calledOnce();
+    });
+  });
+
+});