Code review comment for lp://staging/~hazmat/pyjuju/rapi-login

Revision history for this message
Benjamin Saller (bcsaller) wrote :

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#newcode177
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?

https://codereview.appspot.com/6935068/

« Back to merge proposal