Merge lp:~benji/juju-gui/login into lp:juju-gui/experimental

[Benji York]

Add user login support.

Future work: we need to implement a real UI for this. At the moment this
branch uses prompt() calls to generate modal dialogs. Also, this will
probably need slight tweaking once the backend actually requires
authorization.

https://codereview.appspot.com/7007047/

[Kapil Thangavelu (hazmat)]

needs work

https://codereview.appspot.com/7007047/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode544
app/app.js:544: if (!view.waiting && !view.userIsAuthenticated) {
this content belongs in the app, not the view. the app needs to re-login
on a socket reconnect.

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode547
app/app.js:547: next();
this is an either or proposition, not an and. we either display the
login or the app, not overlay the login over the app, which will be
broken without a working ws.

https://codereview.appspot.com/7007047/diff/1/app/store/env.js
File app/store/env.js (right):

https://codereview.appspot.com/7007047/diff/1/app/store/env.js#newcode77
app/store/env.js:77: this.set('serverReady', true);
instead of introducing another value just move connected down here.

https://codereview.appspot.com/7007047/

[Brad Crittenden (bac)]

This looks ok to me, given the changes Kapil requests. I'll look again
after those changes are made.

https://codereview.appspot.com/7007047/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode530
app/app.js:530: * @param {Object} res ???
Why the duplicate 'res' and the ???

https://codereview.appspot.com/7007047/diff/1/app/views/login.js
File app/views/login.js (right):

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode3
app/views/login.js:3: var no_login_prompts = false;
Why not camelCase?

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode3
app/views/login.js:3: var no_login_prompts = false;
Why not camelCase?

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode27
app/views/login.js:27: * React to the results of sennding a login
message to the server.
typo: sending

https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js
File test/test_login_view.js (right):

https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js#newcode147
test/test_login_view.js:147: // If there are know credentials that are
not known to be bad (they are
s/know/no/

https://codereview.appspot.com/7007047/

[lp:~benji/juju-gui/login]

299. By Benji York on 2013-01-02

checkpoint -- some tests still failing

300. By Benji York on 2013-01-02

merge trunk and fix tests

301. By Benji York on 2013-01-02

checkpoint

302. By Benji York on 2013-01-02

review fixes

[Kapil Thangavelu (hazmat)]

On Wed, Jan 2, 2013 at 2:43 PM, <email address hidden> wrote:

> Please take a look.
>
>
>
> https://codereview.appspot.**com/7007047/diff/1/app/app.js<https://codereview.appspot.com/7007047/diff/1/app/app.js>
> File app/app.js (right):
>
> https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode530<https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode530>
> app/app.js:530: * @param {Object} res ???
> On 2013/01/02 14:45:03, bac wrote:
>
>> Why the duplicate 'res' and the ???
>>
>
> Done.
>
> https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode544<https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode544>
> app/app.js:544: if (!view.waiting && !view.userIsAuthenticated) {
> On 2012/12/21 21:06:18, hazmat wrote:
>
>> this content belongs in the app, not the view. the app needs to
>>
> re-login on a
>
>> socket reconnect.
>>
>
> It appears to me that a socket reconnect is a "reboot" situation for the
> app.
>

i think we have a misunderstanding here, afaics whether or not its a
'reboot' situation has nothing to do with the app's responsibility to hold
on to the credential data or authenticating on the websocket.

 That is, since we can not retry the operation that was ongoing
> when the reconnect happened, we need to restart the app from scratch in
> order to retain consistency. Am I missing a way to retry the last
> operation after a reconnect?
>
>
we can and do track in flight rpc operations, and wait till success ack
before modifying local state. yes, the local state for delta sync data is
effectively reset on reconnect already but that has nothing to do with
storing credentials for authenticating the user again. what's the
concern/usage with retrying operations?

> https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode547<https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode547>
> app/app.js:547: next();
> On 2012/12/21 21:06:18, hazmat wrote:
>
>> this is an either or proposition, not an and. we either display the
>>
> login or the
>
>> app, not overlay the login over the app, which will be broken without
>>
> a working
>
>> ws.
>>
>
> That was my intent, but without a backend that actually requires logins
> I couldn't be sure it is working correctly so I settled on this
> compromise until then.
>
>
the backend requiring the login is irrelevant to the logic that needs to be
done here. the front end app should never toss an error on an
uauthenticated connection. the app knows the connection status and whether
auth has been performed on it, it knows if credentials have been provided
it. If they haven't it displays the login form. If they have it logins the
user in. the user should never see a not authenticated error or the login
form multiple times in a single session (which is incidentally another
reason why the login view shouldn't be persistent).

> https://codereview.appspot.**com/7007047/diff/1/app/store/**env.js<https://codereview.appspot.com/7007047/diff/1/app/store/env.js>
> File app/store/env.js (right):
>

> https://codereview.appspot.**com/7007047/diff/1/app/store/**
> env.js#newcode77<https://codereview.appspot.com/7007047/diff/1/app/store/env.js#ne...

[lp:~benji/juju-gui/login]

303. By Benji York on 2013-01-02

review fix

304. By Benji York on 2013-01-07

checkpoint

305. By Benji York on 2013-01-07

checkpoint

306. By Benji York on 2013-01-07

checkpoint

307. By Benji York on 2013-01-07

merge trunk

308. By Benji York on 2013-01-07

checkpoint

309. By Benji York on 2013-01-07

checkpoint - tests pass

310. By Benji York on 2013-01-07

pre-review fixes

311. By Benji York on 2013-01-07

fix lint

[Gary Poster (gary)]

If you want me to look at this again I would be happy to, but otherwise
land with changes.

Thank you

Gary

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js
File app/store/env.js (right):

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode87
app/store/env.js:87: this.set('defaultSeries', msg.default_series);
The above two lines are supposed to be added to handleLoginEvent, and
removed from here entirely later. Unfortunately, the current rapi
implementation does not support this.

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode113
app/store/env.js:113: }
The implementation does not do what was specced, but we should look
forward to it anyway, I think.

else {
   if (evt.data.provider_type) {
     this.set('providerType', msg.data.provider_type);
   }
   if (evt.data.default_series) {
     this.set('defaultSeries', msg.data.default_series);
   }
}

In the future we should be able to remove the conditional parts of that,
and rely on the two values existing.

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode223
app/store/env.js:223: window.setTimeout(Y.bind(this.login, this), 500);
We talked about this and you liked the idea of connecting this to an
event instead (event fired by login attempt).

https://codereview.appspot.com/7007047/diff/15001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/7007047/diff/15001/test/index.html#newcode37
test/index.html:37: // *after* the test runner is invoked. In which
case the test runner
"In this case,"

https://codereview.appspot.com/7007047/

[Gary Poster (gary)]

Please make a high priority card for the UI implementation, btw

https://codereview.appspot.com/7007047/

[lp:~benji/juju-gui/login]

312. By Benji York on 2013-01-07

review fixes

=== modified file 'app/app.js'
--- app/app.js	2012-12-19 13:45:10 +0000
+++ app/app.js	2013-01-07 21:41:21 +0000
@@ -29,6 +29,11 @@
         preserve: true
       },
 
+      login: {
+        type: 'juju.views.login',
+        preserve: false
+      },
+
       service: {
         type: 'juju.views.service',
         preserve: false,
@@ -238,6 +243,9 @@
       // When the provider type becomes available, display it.
       this.env.after('providerTypeChange', this.onProviderTypeChange);
 
+      // Once the user logs in we need to redraw.
+      this.env.after('login', this.dispatch, this);
+
       // Feed environment changes directly into the database.
       this.env.on('delta', this.db.on_delta, this.db);
 
@@ -517,6 +525,45 @@
     },
 
     /**
+     * Ensure that the current user has authenticated.
+     *
+     * @method check_user_credentials
+     * @param {Object} req The request.
+     * @param {Object} res ???
+     * @param {Object} next The next route handler.
+     *
+     */
+    check_user_credentials: function(req, res, next) {
+      var viewInfo = this.getViewInfo('login');
+      if (!viewInfo.instance) {
+        viewInfo.instance = new views.LoginView({
+          app: this,
+          env: this.env
+        });
+      }
+      var view = viewInfo.instance;
+      // If there are no stored credentials the user is prompted for some.
+      var user = this.env.get('user');
+      var password = this.env.get('password');
+      if (!Y.Lang.isValue(user) || !Y.Lang.isValue(password)) {
+        view.promptUser();
+      }
+      // If there are credentials available and there has not been a successful
+      // login attempt and we are not waiting on a login attempt, try to log in.
+      if (Y.Lang.isValue(user) && Y.Lang.isValue(password) &&
+          !this.env.waiting && !this.env.userIsAuthenticated) {
+        this.env.login();
+        return;
+      }
+      // If there has not been a successful login attempt, do not let the route
+      // dispatch proceed.
+      if (this.env.waiting || !this.env.userIsAuthenticated) {
+        return;
+      }
+      next();
+    },
+
+    /**
      * Display the provider type.
      *
      * The provider type arrives asynchronously.  Instead of updating the
@@ -709,6 +756,7 @@
       charm_store: {},
       routes: {
         value: [
+          { path: '*', callback: 'check_user_credentials'},
           { path: '*', callback: 'show_notifications_view'},
           { path: '/charms/', callback: 'show_charm_collection'},
           { path: '/charms/*charm_store_path',
@@ -746,6 +794,7 @@
 }, '0.5.2', {
   requires: [
     'juju-models',
+    'juju-notification-controller',
     'juju-charm-models',
     'juju-views',
     'juju-controllers',

=== modified file 'app/modules-debug.js'
--- app/modules-debug.js	2012-12-21 12:52:30 +0000
+++ app/modules-debug.js	2013-01-07 21:41:21 +0000
@@ -96,6 +96,10 @@
           fullpath: '/juju-ui/views/environment.js'
         },
 
+        'juju-view-login': {
+          fullpath: '/juju-ui/views/login.js'
+        },
+
         'juju-view-service': {
           fullpath: '/juju-ui/views/service.js'
         },
@@ -124,6 +128,7 @@
             'juju-view-utils',
             'juju-topology',
             'juju-view-environment',
+            'juju-view-login',
             'juju-view-service',
             'juju-view-unit',
             'juju-view-charm',

=== modified file 'app/modules-prod.js'
--- app/modules-prod.js	2012-12-13 02:43:15 +0000
+++ app/modules-prod.js	2013-01-07 21:41:21 +0000
@@ -17,6 +17,7 @@
             'juju-topology',
             'juju-view-utils',
             'juju-view-environment',
+            'juju-view-login',
             'juju-view-service',
             'juju-view-unit',
             'juju-view-charm',

=== modified file 'app/store/env.js'
--- app/store/env.js	2012-10-15 13:20:38 +0000
+++ app/store/env.js	2013-01-07 21:41:21 +0000
@@ -1,5 +1,8 @@
 'use strict';
 
+// A global.
+var noLogin;
+
 YUI.add('juju-env', function(Y) {
 
   function Environment(config) {
@@ -12,6 +15,8 @@
   Environment.ATTRS = {
     'socket_url': {},
     'conn': {},
+    'user': {},
+    'password': {},
     'connected': {value: false},
     'debug': {value: false}
   };
@@ -30,6 +35,11 @@
       this._counter = 0;
       // mapping txn-id callback if any.
       this._txn_callbacks = {};
+      // Consider the user unauthenticated until proven otherwise.
+      this.userIsAuthenticated = false;
+      // When the server tells us the outcome of a login attempt we record
+      // the result.
+      this.on('login', this.handleLoginEvent, this);
     },
 
     destructor: function() {
@@ -58,7 +68,6 @@
 
     on_open: function(data) {
       console.log('Env: Connected');
-      this.set('connected', true);
     },
 
     on_close: function(data) {
@@ -68,21 +77,37 @@
 
     on_message: function(evt) {
       console.log('Env: Receive', evt.data);
-
       var msg = Y.JSON.parse(evt.data);
+      // The "ready" attribute indicates that this is a server's initial
+      // greeting.  It provides a few initial values that we care about.
       if (msg.ready) {
-        // The "ready" attribute indicates that this is a server's initial
-        // greeting.  It provides a few initial values that we care about.
+        console.log('Env: Handshake Complete');
+        this.set('connected', true);
         this.set('providerType', msg.provider_type);
         this.set('defaultSeries', msg.default_series);
-      }
-      if (msg.version === 0) {
-        console.log('Env: Handshake Complete');
         return;
       }
       this.fire('msg', msg);
     },
 
+    /**
+     * React to the results of sending a login message to the server.
+     *
+     * @method handleLoginEvent
+     * @param {Object} evt The event to which we are responding.
+     * @return {undefined} Nothing.
+     */
+    handleLoginEvent: function(evt) {
+      // We are only interested in the responses to login events.
+      this.userIsAuthenticated = !!evt.data.result;
+      this.waiting = false;
+      // If the credentials were rejected remove them.
+      if (!this.userIsAuthenticated) {
+        this.set('user', undefined);
+        this.set('password', undefined);
+      }
+    },
+
     dispatch_result: function(data) {
       console.log('Env: Dispatch Result', data);
       this._dispatch_rpc_result(data);
@@ -174,6 +199,23 @@
       this._send_rpc({'op': 'expose', 'service_name': service}, callback);
     },
 
+    /**
+     * Attempt to log the user in.  Credentials must have been previously
+     * stored on the environment.  If not, this method will schedule a call to
+     * itself in the future in order to try again.
+     *
+     * @return {undefined} Nothing.
+     */
+    login: function() {
+      // If the user is already authenticated there is nothing to do.
+      if (this.userIsAuthenticated) {
+        return;
+      }
+      var user = this.get('user');
+      var password = this.get('password');
+      this._send_rpc({op: 'login', user: user, password: password});
+    },
+
     unexpose: function(service, callback) {
       this._send_rpc({'op': 'unexpose', 'service_name': service}, callback);
     },

=== added file 'app/views/login.js'
--- app/views/login.js	1970-01-01 00:00:00 +0000
+++ app/views/login.js	2013-01-07 21:41:21 +0000
@@ -0,0 +1,59 @@
+'use strict';
+
+// Should login prompts be presented?  Turned on for testing.
+var noLogin = noLogin || false;
+
+YUI.add('juju-view-login', function(Y) {
+
+  var views = Y.namespace('juju.views');
+  var Templates = views.Templates;
+
+  var LoginView = Y.Base.create('LoginView', Y.View, [views.JujuBaseView], {
+    // This is so tests can easily determine if the user was prompted.
+    _prompted: false,
+
+    /**
+     * Prompt the user for input.
+     *
+     * Does nothing if a special global flag has been set.  This is used so
+     * tests do not generate prompts.
+     *
+     * @method _prompt
+     * @param {String} message The message to display to the user.
+     * @return {String} The string the user typed.
+     */
+    _prompt: function(message) {
+      // noLogin is a global.
+      if (noLogin) {
+        return null;
+      }
+      return window.prompt(message);
+    },
+
+    /**
+     * Prompt the user for their user name and password.
+     *
+     * @method promptUser
+     * @return {undefined} Nothing.
+     */
+    promptUser: function() {
+      this._prompted = true;
+      var env = this.get('env');
+      // The user's name is always "admin".
+      env.set('user', 'admin');
+      env.set('password', this._prompt('Password'));
+    }
+
+  });
+
+  views.LoginView = LoginView;
+
+}, '0.1.0', {
+  requires: [
+    'view',
+    'juju-view-utils',
+    'node',
+    'handlebars'
+  ]
+});
+

=== modified file 'test/index.html'
--- test/index.html	2012-12-21 12:06:26 +0000
+++ test/index.html	2013-01-07 21:41:21 +0000
@@ -13,10 +13,34 @@
   <script src="assets/mocha.js"></script>
   <script src="utils.js"></script>
   <script>
+    noLogin = true;
     var assert = chai.assert,
         expect = chai.expect
         should = chai.should();
-    mocha.setup({'ui': 'bdd', 'ignoreLeaks': false})
+    mocha.setup({ui: 'bdd', ignoreLeaks: false})
+  </script>
+
+  <script>
+  YUI().use('node', 'event', function(Y) {
+     var config = GlobalConfig;
+     for (group in config.groups) {
+       var group = config.groups[group];
+       for (m in group.modules) {
+         var resource = group.modules[m];
+         if (!m || !resource.fullpath) {
+           continue
+         }
+         resource.fullpath = resource.fullpath.replace(
+           '/juju-ui/', '../juju-ui/', 1);
+         // If we load modules asyncronously then the module loading may take
+         // so long that the test definitions (and before/after calls) happen
+         // *after* the test runner is invoked.  In this case the test runner
+         // will not know about the tests and therefore not run them.
+         resource.async = false;
+       }
+     }
+     Y.on('domready', mocha.run);
+  });
   </script>
 
   <script src="test_d3_components.js"></script>
@@ -33,6 +57,7 @@
   <script src="test_service_config_view.js"></script>
   <script src="test_service_view.js"></script>
   <script src="test_utils.js"></script>
+  <script src="test_login.js"></script>
   <script src="test_charm_panel.js"></script>
   <script src="test_charm_configuration.js"></script>
   <script src="test_console.js"></script>
@@ -42,27 +67,6 @@
   <script src="test_app_hotkeys.js"></script>
   <script src="test_notifier_widget.js"></script>
 
-  <script>
-  YUI().use('node', 'event', function(Y) {
-     Y.on('domready', function() {
-
-     var config = GlobalConfig;
-     for (group in config.groups) {
-          var group = config.groups[group];
-         for (m in group.modules) {
-            var resource = group.modules[m];
-            if (!m || !resource.fullpath) {
-              continue
-            }
-            resource.fullpath = resource.fullpath.replace(
-              '/juju-ui/', '../juju-ui/', 1);
-         }
-     }
-     // Load before test runner
-     mocha.run();
-     });
-  });
-  </script>
 
 </head>
 

=== modified file 'test/test_app.js'
--- test/test_app.js	2012-12-03 20:24:44 +0000
+++ test/test_app.js	2013-01-07 21:41:21 +0000
@@ -119,6 +119,7 @@
 });
 
 YUI(GlobalConfig).use(['juju-gui', 'juju-tests-utils'], function(Y) {
+
   describe('Application Connection State', function() {
     var container;
 
@@ -128,7 +129,7 @@
 
     it('should be able to handle env connection status changes', function() {
       var juju = Y.namespace('juju'),
-          conn = new(Y.namespace('juju-tests.utils')).SocketStub(),
+          conn = new (Y.namespace('juju-tests.utils')).SocketStub(),
           env = new juju.Environment({conn: conn}),
           app = new Y.juju.App({env: env, container: container}),
           reset_called = false,
@@ -153,11 +154,14 @@
       };
       env.connect();
       conn.open();
+      // We need to fake the connection event.
+      env.set('connected', true);
       reset_called.should.equal(true);
 
       // trigger a second time and verify
       reset_called = false;
       conn.open();
+      env.set('connected', true);
       reset_called.should.equal(true);
     });
 
@@ -229,6 +233,8 @@
       env.get_endpoints = function(services, callback) {
         get_endpoints_count += 1;
       };
+      // We need to fake the connection event.
+      env.set('connected', true);
       // Inject default data, should only get_endpoints once.
       injectData(app);
       get_endpoints_count.should.equal(1);

=== modified file 'test/test_app_hotkeys.js'
--- test/test_app_hotkeys.js	2012-11-27 14:25:32 +0000
+++ test/test_app_hotkeys.js	2013-01-07 21:41:21 +0000
@@ -9,7 +9,8 @@
           var env = {
             after: function() {},
             get: function() {},
-            on: function() {}
+            on: function() {},
+            set: function() {}
           };
           windowNode = Y.one(window);
           app = new Y.juju.App({

=== added file 'test/test_login.js'
--- test/test_login.js	1970-01-01 00:00:00 +0000
+++ test/test_login.js	2013-01-07 21:41:21 +0000
@@ -0,0 +1,86 @@
+'use strict';
+
+(function() {
+
+  var requires = ['node', 'juju-gui', 'juju-views', 'juju-tests-utils'];
+  var Y = YUI(GlobalConfig).use(requires);
+
+  describe('environment login support', function() {
+    var conn, env, utils, juju, makeLoginView, views, app;
+    var test = it; // We aren't really doing BDD so let's be more direct.
+
+    before(function() {
+      utils = Y.namespace('juju-tests.utils');
+      juju = Y.namespace('juju');
+    });
+
+    beforeEach(function() {
+      var container = Y.Node.create('<div/>');
+      conn = new utils.SocketStub();
+      env = new juju.Environment({conn: conn});
+      env.connect();
+      conn.open();
+      //      env.set('connected', true);
+    });
+
+    afterEach(function() {
+      env.destroy();
+    });
+
+    test('the user is initially assumed to be unauthenticated', function() {
+      assert.equal(env.userIsAuthenticated, false);
+    });
+
+    test('successful login event marks user as authenticated', function() {
+      var evt = {data: {op: 'login', result: true}};
+      env.handleLoginEvent(evt);
+      assert.equal(env.userIsAuthenticated, true);
+    });
+
+    test('unsuccessful login event keeps user unauthenticated', function() {
+      var evt = {data: {op: 'login'}};
+      env.handleLoginEvent(evt);
+      assert.equal(env.userIsAuthenticated, false);
+    });
+
+    test('bad credentials are removed', function() {
+      var evt = {data: {op: 'login'}};
+      env.handleLoginEvent(evt);
+      assert.equal(env.get('user'), undefined);
+      assert.equal(env.get('password'), undefined);
+    });
+
+    test('credentials passed to the constructor are stored', function() {
+      var user = 'Will Smith';
+      var password = 'I am legend!';
+      var env = new juju.Environment({
+        user: user,
+        password: password,
+        conn: conn
+      });
+      assert.equal(env.get('user'), user);
+      assert.equal(env.get('password'), password);
+    });
+
+    test('login requests are sent in response to a connection', function() {
+      env.fire('log', {op: 'login', data: {}});
+    });
+
+    test('if already authenticated, login() is a no-op', function() {
+      env.userIsAuthenticated = true;
+      env.login();
+      assert.equal(conn.last_message(), undefined);
+    });
+
+    test('with credentials set, login() sends an RPC message', function() {
+      env.set('user', 'user');
+      env.set('password', 'password');
+      env.login();
+      assert.equal(conn.last_message().op, 'login');
+      assert.equal(conn.last_message().user, 'user');
+      assert.equal(conn.last_message().password, 'password');
+    });
+
+  });
+
+})();

=== modified file 'test/test_service_view.js'
--- test/test_service_view.js	2012-11-08 18:16:07 +0000
+++ test/test_service_view.js	2013-01-07 21:41:21 +0000
@@ -23,7 +23,8 @@
       env = new (Y.namespace('juju')).Environment({conn: conn});
       env.connect();
       conn.open();
-      container = Y.Node.create('<div id="test-container" />');
+      container = Y.Node.create('<div/>')
+        .hide();
       Y.one('#main').append(container);
       db = new models.Database();
       charm = new models.Charm({id: 'cs:precise/mysql-7', description: 'A DB'});