diff --git a/etcdserver/auth/auth.go b/etcdserver/auth/auth.go index e59edcbc0..272f2d370 100644 --- a/etcdserver/auth/auth.go +++ b/etcdserver/auth/auth.go @@ -281,13 +281,7 @@ func (s *store) UpdateUser(user User) (User, error) { return old, err } - hash, err := s.HashPassword(user.Password) - if err != nil { - return old, err - } - user.Password = hash - - newUser, err := old.merge(user) + newUser, err := old.merge(user, s.PasswordStore) if err != nil { return old, err } @@ -448,29 +442,33 @@ func (s *store) DisableAuth() error { // is called and returns a new User with these modifications applied. Think of // all Users as immutable sets of data. Merge allows you to perform the set // operations (desired grants and revokes) atomically -func (u User) merge(n User) (User, error) { +func (ou User) merge(nu User, s PasswordStore) (User, error) { var out User - if u.User != n.User { - return out, authErr(http.StatusConflict, "Merging user data with conflicting usernames: %s %s", u.User, n.User) + if ou.User != nu.User { + return out, authErr(http.StatusConflict, "Merging user data with conflicting usernames: %s %s", ou.User, nu.User) } - out.User = u.User - if n.Password != "" { - out.Password = n.Password + out.User = ou.User + if nu.Password != "" { + hash, err := s.HashPassword(nu.Password) + if err != nil { + return ou, err + } + out.Password = hash } else { - out.Password = u.Password + out.Password = ou.Password } - currentRoles := types.NewUnsafeSet(u.Roles...) - for _, g := range n.Grant { + currentRoles := types.NewUnsafeSet(ou.Roles...) + for _, g := range nu.Grant { if currentRoles.Contains(g) { - plog.Noticef("granting duplicate role %s for user %s", g, n.User) - return User{}, authErr(http.StatusConflict, fmt.Sprintf("Granting duplicate role %s for user %s", g, n.User)) + plog.Noticef("granting duplicate role %s for user %s", g, nu.User) + return User{}, authErr(http.StatusConflict, fmt.Sprintf("Granting duplicate role %s for user %s", g, nu.User)) } currentRoles.Add(g) } - for _, r := range n.Revoke { + for _, r := range nu.Revoke { if !currentRoles.Contains(r) { - plog.Noticef("revoking ungranted role %s for user %s", r, n.User) - return User{}, authErr(http.StatusConflict, fmt.Sprintf("Revoking ungranted role %s for user %s", r, n.User)) + plog.Noticef("revoking ungranted role %s for user %s", r, nu.User) + return User{}, authErr(http.StatusConflict, fmt.Sprintf("Revoking ungranted role %s for user %s", r, nu.User)) } currentRoles.Remove(r) } diff --git a/etcdserver/auth/auth_test.go b/etcdserver/auth/auth_test.go index a28a98948..17e91ba23 100644 --- a/etcdserver/auth/auth_test.go +++ b/etcdserver/auth/auth_test.go @@ -26,6 +26,21 @@ import ( "golang.org/x/net/context" ) +type fakeDoer struct{} + +func (_ fakeDoer) Do(context.Context, etcdserverpb.Request) (etcdserver.Response, error) { + return etcdserver.Response{}, nil +} + +func TestCheckPassword(t *testing.T) { + st := NewStore(fakeDoer{}, 5*time.Second) + u := User{Password: "$2a$10$I3iddh1D..EIOXXQtsra4u8AjOtgEa2ERxVvYGfXFBJDo1omXwP.q"} + matched := st.CheckPassword(u, "foo") + if matched { + t.Fatalf("expected false, got %v", matched) + } +} + const testTimeout = time.Millisecond func TestMergeUser(t *testing.T) { @@ -71,16 +86,16 @@ func TestMergeUser(t *testing.T) { User{User: "foo", Roles: []string{"role1", "role2"}}, false, }, - { - User{User: "foo"}, - User{User: "foo", Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"}, - User{User: "foo", Roles: []string{}, Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"}, + { // empty password will not overwrite the previous password + User{User: "foo", Password: "foo", Roles: []string{}}, + User{User: "foo", Password: ""}, + User{User: "foo", Password: "foo", Roles: []string{}}, false, }, } for i, tt := range tbl { - out, err := tt.input.merge(tt.merge) + out, err := tt.input.merge(tt.merge, passwordStore{}) if err != nil && !tt.iserr { t.Fatalf("Got unexpected error on item %d", i) }