state: add SetPassword methods

state: add SetPassword methods



https://code.launchpad.net/~rogpeppe/juju-core/096-state-set-password/+merge/127530

(do not edit description out of merge proposal)

[rog, 2012-10-02 16:32:23]

Please take a look.

[niemeyer, 2012-10-02 17:02:06]

Looking nice. Just needs a test to ensure the bug is fixed, and a future CL
needs to drop the password from the database at appropriate times.

https://codereview.appspot.com/6587060/diff/2001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6587060/diff/2001/state/open.go#newcode32
state/open.go:32: Entity string
If we have it as "EntityName" everywhere else, feels like a good idea to have it
here as well.

https://codereview.appspot.com/6587060/diff/2001/state/open.go#newcode123
state/open.go:123: if err := db.Login(entity, password); err != nil {
A test is needed to validate that the presence database is in fact
authenticated.

[rog, 2012-10-03 17:50:43]

Please take a look.

[rog, 2012-10-03 17:52:49]

https://codereview.appspot.com/6587060/diff/2001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6587060/diff/2001/state/open.go#newcode123
state/open.go:123: if err := db.Login(entity, password); err != nil {
On 2012/10/02 17:02:06, niemeyer wrote:
> A test is needed to validate that the presence database is in fact
> authenticated.

done. (i checked that leaving this as db not pdb causes tests to fail).

[niemeyer, 2012-10-03 18:11:15]

That's very nice.

I'd just change the Info.EntityName logic so that empty means admin, otherwise
LGTM.

https://codereview.appspot.com/6587060/diff/7005/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6587060/diff/7005/state/open.go#newcode30
state/open.go:30: // This may be "admin" to connect as the administrator.
I suggest instead:

// EntityName holds the name of the connecting entity.
// It should be empty when connecting as an administrator.

and

// Password holds the password for the administrator or named entity.

https://codereview.appspot.com/6587060/diff/7005/state/open.go#newcode123
state/open.go:123: if err.Error() == "auth fails" {
Don't we have a code in those cases? Can we have a comment here saying which
case that is? Comparing errors by string is not ideal, so we should leave some
breadcrumbs about the case.

https://codereview.appspot.com/6587060/diff/7005/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6587060/diff/7005/state/state_test.go#newcode897
state/state_test.go:897: func (s *StateSuite) TestOpenWithoutSetPassword(c *C) {
Very nice tests.

https://codereview.appspot.com/6587060/diff/7005/state/state_test.go#newcode949
state/state_test.go:949: ent, err = getEntity(st)
c.Assert(err, IsNil)

[rog, 2012-10-04 07:39:44]

*** Submitted:

state: add SetPassword methods

R=niemeyer
CC=
https://codereview.appspot.com/6587060

https://codereview.appspot.com/6587060/diff/7005/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6587060/diff/7005/state/open.go#newcode30
state/open.go:30: // This may be "admin" to connect as the administrator.
On 2012/10/03 18:11:15, niemeyer wrote:
> I suggest instead:
> 
> // EntityName holds the name of the connecting entity.
> // It should be empty when connecting as an administrator.
> 
> and
> 
> // Password holds the password for the administrator or named entity.

Done.

https://codereview.appspot.com/6587060/diff/7005/state/open.go#newcode123
state/open.go:123: if err.Error() == "auth fails" {
On 2012/10/03 18:11:15, niemeyer wrote:
> Don't we have a code in those cases? Can we have a comment here saying which
> case that is? Comparing errors by string is not ideal, so we should leave some
> breadcrumbs about the case.

No code, I'm afraid. Done, I think.

https://codereview.appspot.com/6587060/diff/7005/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6587060/diff/7005/state/state_test.go#newcode949
state/state_test.go:949: ent, err = getEntity(st)
On 2012/10/03 18:11:15, niemeyer wrote:
> c.Assert(err, IsNil)

Done.