Require login before api usage.

Require login before api usage.

This update requires login before using the ws api and before the delta stream is active.

https://code.launchpad.net/~hazmat/juju/rapi-login/+merge/140268

(do not edit description out of merge proposal)

[hazmat, 2012-12-17 18:01:31]

Please take a look.

[teknico, 2012-12-17 18:16:45]

Code looks good, on the face of it (I'm not familiar with this codebase).

I dislike one- or two-letter names, even in tests, but that's just personal
preference.

[benjamin.saller, 2012-12-17 18:36:59]

I think I'm a little shy on context here given the comments below. If the goals
are in keeping with the comments then this seems ok, if we are looking for real
ACL access over the tree, it wasn't clear to make that worked as expected.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py
File juju/agents/api.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode102
juju/agents/api.py:102: break
Its not possible for someone to register another 'admin' like name here or will
the first added always appear first? This seems an odd way of tracking this.
Since this codebase only supports admin anyway wouldn't this be acls[0] anyway
or are there others?

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode105
juju/agents/api.py:105: "/login", acls=[make_ace(admin_acl['id'], all=True)])
I don't recall the details of the security branch, but this copies it from
/initialized to /login basically? I only have vague guesses as to why this is
the process, you need to create it during init to set up the agents and then add
it to the login tree which will in the future support additional logins?

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py#newcode222
juju/rapi/context.py:222: yield self.client.get("/login")
This does the ACL check on the login node and and uses that as the success
boolean. Are the other nodes protected with the same ACL? Its not clear where
that's being managed here.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py
File juju/rapi/tests/test_context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py...
juju/rapi/tests/test_context.py:177: 
Again it looks like the ACL check works here, but I'm not sure if the way this
is wired in provides any actual access control outside of that node. Is that a
non-goal with this iteration?

[hazmat, 2012-12-17 18:53:51]

replies to ben's comments.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py
File juju/agents/api.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode102
juju/agents/api.py:102: break
On 2012/12/17 18:36:59, benjamin.saller wrote:
> Its not possible for someone to register another 'admin' like name here or
will
> the first added always appear first? This seems an odd way of tracking this.
> Since this codebase only supports admin anyway wouldn't this be acls[0] anyway
> or are there others?

at the moment the only time this node is created is if it doesn't exist, at the
client side. there is no other client that will modify it once its in place. the
initialized node is the marker node for the rest of the system to go forward
once the environment details have been loaded.

ie. while its true that multiple 'admin' users could potentially be created with
different passwords, this node is WORM and thus suitable for this use.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode105
juju/agents/api.py:105: "/login", acls=[make_ace(admin_acl['id'], all=True)])
On 2012/12/17 18:36:59, benjamin.saller wrote:
> I don't recall the details of the security branch, but this copies it from
> /initialized to /login basically? 

This copies the admin identity (which is a hash of the secret of the user id)
with a more restrictive acl grant to a newly created login node. Effectively to
read this node you must have authenticated with the admin credentials.

This code also exists in initialized, but our current usage/deployment mode is
via charm so the initialize code from this branch will never be called, hence
this lazy init technique for the api agent startup.


> I only have vague guesses as to why this is
> the process, you need to create it during init to set up the agents and then
add
> it to the login tree which will in the future support additional logins?

hopefully the previous comments make it clear.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py#newcode222
juju/rapi/context.py:222: yield self.client.get("/login")
On 2012/12/17 18:36:59, benjamin.saller wrote:
> This does the ACL check on the login node and and uses that as the success
> boolean. Are the other nodes protected with the same ACL? Its not clear where
> that's being managed here.

access to the node is only possible if the username and password are correct,
ie. match the admin identity used for the node acl.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py
File juju/rapi/tests/test_context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py...
juju/rapi/tests/test_context.py:177: 
On 2012/12/17 18:36:59, benjamin.saller wrote:
> Again it looks like the ACL check works here, but I'm not sure if the way this
> is wired in provides any actual access control outside of that node. Is that 
non-goal with this iteration?

this isn't about access control at the node tree level, that's the security
branch work which is *much* larger in scope. this is about providing a mechanism
to login for the browser and securing the api endpoint. a followup branch to
rapi once the login is incorporated to the gui, will disable rapi usage till the
client is authenticated. its not being done here, to prevent breakage across the
developer team and staging site, till the gui has login capabilities.

[hazmat, 2013-01-17 23:17:47]

Please take a look.

[hazmat, 2013-01-17 23:20:53]

Please take a look.

[bcsaller, 2013-01-18 14:04:23]

LGTM

I didn't see a test around the login call returning the added unauthorized error
though.

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/context.py#newcode85
juju/rapi/context.py:85: if not self.authenticated and not func.__name__ ==
"_login":
func.__name__ has a bad smell to it but fair enough

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py
File juju/rapi/transport/ws.py (right):

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py#n...
juju/rapi/transport/ws.py:68: 
Is there an use case for a logout/inverse to this stopping the delta? I suspect
not at this time.

[hazmat, 2013-01-18 15:14:53]

thanks for the review

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/context.py#newcode85
juju/rapi/context.py:85: if not self.authenticated and not func.__name__ ==
"_login":
On 2013/01/18 14:04:23, bcsaller wrote:
> func.__name__ has a bad smell to it but fair enough

its already playing with func.. not many other options outside of decorating
every other function with another attribute for perm, which still amounts to the
same, ie func inspection.

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py
File juju/rapi/transport/ws.py (right):

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py#n...
juju/rapi/transport/ws.py:68: 
On 2013/01/18 14:04:23, bcsaller wrote:
> Is there an use case for a logout/inverse to this stopping the delta? I
suspect
> not at this time.

logout not possible with the underlying zk auth mechanism, wrt to use not at the
moment outside of disconnect.

ideally stream sub/unsub would be explicit api methods, but doing so without gui
support first would break the gui.