Mercurial > hg > thermostat-ng > web-client
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(); + }); + }); + +});