Fully integrate AuthenticationTokenHandler

Fully integrate AuthenticationTokenHandler

This might be the last of the charm branches for the authtoken feature.  It hooks up the handler and tests the integration.

https://code.launchpad.net/~gary/charms/precise/juju-gui/authtoken3/+merge/196414

Requires: https://code.launchpad.net/~gary/charms/precise/juju-gui/authtoken2/+merge/196351

(do not edit description out of merge proposal)

[gary.poster, 2013-11-23 02:22:17]

Please take a look.

[gary.poster, 2013-11-23 02:57:58]

Please take a look.

[gary.poster, 2013-11-23 03:16:40]

Please take a look.

[frankban, 2013-11-25 11:59:34]

Very nice branch Gary, thank you!
LGTM (with some comments below), and QA ok!
To QA this branch I deployed the charm in a local environment using "make
deploy" and then I successfully run this script:
http://pastebin.ubuntu.com/6473403/

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py
File server/guiserver/auth.py (right):

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:27: - login_succeeded(data) -> bool.
- make_request(request_id, username, password) -> dict

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:29: requests' data based on the API implementation
currently in use. Backends
They can also be used to create authentication requests... or similar?

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:36: if the authentication succeeds.
Since this docstring summarizes the purposes of objects in this module, it would
be nice to provide some info about the AuthenticationTokenHandler here.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:198: def make_request(self, request_id, username,
password):
"""Create and return an authentication request."""? <shrug>

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:349: def expire_token():
I know this is pre-existing code, but what do you think about logging the token
expiration here?

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:385: if credentials is not None:
... and maybe here we could log the fact that someone is actually using the
token?

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/handlers.py
File server/guiserver/handlers.py (right):

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/handlers....
server/guiserver/handlers.py:157: elif self.tokens.token_requested(data):
A token request is only handled by the guiserver if the user is authenticated.
This means that a GUIToken Create request from an "anonymous" client is
propagated to juju-core, and the response will be something like this:
{
 'RequestId': 0, 
 'Error': 'unknown object type "GUIToken"',
 'Response': {},
}
This can be ok but seems confusing from the client perspective; what we might
want to send back is probably something like "unauthorized access: no user
logged in".
The quick fix is to always check if a token is requested (s/elif/if, similar to
what already done with deployer requests at line 145) and ensure the user is
authenticated in the process_token_request method itself, returning a response
or an error accordingly. Of course this can be done (if you agree with the
proposed change) in another branch.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/tests/tes...
File server/guiserver/tests/test_auth.py (right):

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/tests/tes...
server/guiserver/tests/test_auth.py:151: def test_token_login_request(self):
Great tests!

[gary.poster, 2013-11-25 14:03:20]

Please take a look.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py
File server/guiserver/auth.py (right):

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:27: - login_succeeded(data) -> bool.
On 2013/11/25 11:59:34, frankban wrote:
> - make_request(request_id, username, password) -> dict

Done.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:29: requests' data based on the API implementation
currently in use. Backends
On 2013/11/25 11:59:34, frankban wrote:
> They can also be used to create authentication requests... or similar?

Done.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:36: if the authentication succeeds.
On 2013/11/25 11:59:34, frankban wrote:
> Since this docstring summarizes the purposes of objects in this module, it
would
> be nice to provide some info about the AuthenticationTokenHandler here.

Done.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:198: def make_request(self, request_id, username,
password):
On 2013/11/25 11:59:34, frankban wrote:
> """Create and return an authentication request."""? <shrug>

Done.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:349: def expire_token():
On 2013/11/25 11:59:34, frankban wrote:
> I know this is pre-existing code, but what do you think about logging the
token
> expiration here?

Done.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/auth.py#n...
server/guiserver/auth.py:385: if credentials is not None:
On 2013/11/25 11:59:34, frankban wrote:
> ... and maybe here we could log the fact that someone is actually using the
> token?

Done.

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/handlers.py
File server/guiserver/handlers.py (right):

https://codereview.appspot.com/31290043/diff/30001/server/guiserver/handlers....
server/guiserver/handlers.py:157: elif self.tokens.token_requested(data):
On 2013/11/25 11:59:34, frankban wrote:
> A token request is only handled by the guiserver if the user is authenticated.
> This means that a GUIToken Create request from an "anonymous" client is
> propagated to juju-core, and the response will be something like this:
> {
>  'RequestId': 0, 
>  'Error': 'unknown object type "GUIToken"',
>  'Response': {},
> }
> This can be ok but seems confusing from the client perspective; what we might
> want to send back is probably something like "unauthorized access: no user
> logged in".
> The quick fix is to always check if a token is requested (s/elif/if, similar
to
> what already done with deployer requests at line 145) and ensure the user is
> authenticated in the process_token_request method itself, returning a response
> or an error accordingly. Of course this can be done (if you agree with the
> proposed change) in another branch.

Good idea, thank you.  I went ahead and did it.

[frankban, 2013-11-25 14:14:22]

Changes LGTM, thank you!

[gary.poster, 2013-11-25 14:18:25]

*** Submitted:

Fully integrate AuthenticationTokenHandler

This might be the last of the charm branches for the authtoken feature.  It
hooks up the handler and tests the integration.

R=frankban
CC=
https://codereview.appspot.com/31290043

[gary.poster, 2013-11-25 14:20:03]

Thanks for the fantastic review(s), and for going above and beyond with the QA,
Francesco.