diff --git a/app/admin/models/sys_user.go b/app/admin/models/sys_user.go index eab2cadf..44dd578d 100644 --- a/app/admin/models/sys_user.go +++ b/app/admin/models/sys_user.go @@ -42,19 +42,35 @@ func (e *SysUser) GetId() interface{} { return e.UserId } -// Encrypt 加密 -func (e *SysUser) Encrypt() (err error) { +// Encrypt hashes Password, unless it already holds a hash. +// +// The hooks below run on whatever is in the struct, and a user read from the +// database carries the stored hash in that field. Hashing it again produces a +// hash of a hash, and the password that user knows no longer matches anything: +// they cannot log in, and nothing reports an error. The only thing preventing +// that today is an Omit("password") on the one update that loads a user first, +// which makes every other write to this model one line away from destroying +// credentials. +// +// bcrypt.Cost parses a hash and fails on anything else, so it distinguishes +// the two cases without the call site having to say which it is. The cost is +// that a password which is itself a well-formed bcrypt hash would be stored +// unchanged - a 60-character string beginning "$2a$", not something a person +// types, and it grants whoever set it no access they did not already have. +func (e *SysUser) Encrypt() error { if e.Password == "" { - return + return nil + } + if _, err := bcrypt.Cost([]byte(e.Password)); err == nil { + return nil } - var hash []byte - if hash, err = bcrypt.GenerateFromPassword([]byte(e.Password), bcrypt.DefaultCost); err != nil { - return - } else { - e.Password = string(hash) - return + hash, err := bcrypt.GenerateFromPassword([]byte(e.Password), bcrypt.DefaultCost) + if err != nil { + return err } + e.Password = string(hash) + return nil } func (e *SysUser) BeforeCreate(_ *gorm.DB) error { @@ -62,11 +78,7 @@ func (e *SysUser) BeforeCreate(_ *gorm.DB) error { } func (e *SysUser) BeforeUpdate(_ *gorm.DB) error { - var err error - if e.Password != "" { - err = e.Encrypt() - } - return err + return e.Encrypt() } func (e *SysUser) AfterFind(_ *gorm.DB) error { diff --git a/app/admin/models/sys_user_password_test.go b/app/admin/models/sys_user_password_test.go new file mode 100644 index 00000000..cdfd5dca --- /dev/null +++ b/app/admin/models/sys_user_password_test.go @@ -0,0 +1,106 @@ +package models + +import ( + "testing" + + "golang.org/x/crypto/bcrypt" +) + +const knownPassword = "correct-horse-battery-staple" + +// A user loaded from the database carries the stored hash in Password, and the +// hooks run on whatever is in the struct. Hashing it a second time produces a +// hash of a hash: the password the user knows stops matching, they cannot log +// in, and nothing reports an error. +// +// Only an Omit("password") on one call site stood between this and every write +// to the model. This is the test that removes the need for it. +func TestEncryptLeavesAnAlreadyHashedPasswordAlone(t *testing.T) { + fresh := SysUser{Password: knownPassword} + if err := fresh.Encrypt(); err != nil { + t.Fatalf("Encrypt: %v", err) + } + stored := fresh.Password + if err := bcrypt.CompareHashAndPassword([]byte(stored), []byte(knownPassword)); err != nil { + t.Fatalf("setup failed: the password was not hashed: %v", err) + } + + // What a query puts in the struct, and what an update then hands the hook. + loaded := SysUser{Password: stored} + if err := loaded.Encrypt(); err != nil { + t.Fatalf("Encrypt on a loaded user: %v", err) + } + if loaded.Password != stored { + t.Error("Encrypt re-hashed a stored hash; the user can no longer log in") + } + if err := bcrypt.CompareHashAndPassword([]byte(loaded.Password), []byte(knownPassword)); err != nil { + t.Errorf("the user can no longer log in with their password: %v", err) + } +} + +// The other half: a password that is not a hash still gets hashed, on create +// and on update alike. +func TestEncryptHashesAPlaintextPassword(t *testing.T) { + for _, c := range []struct { + name string + hook func(*SysUser) error + }{ + {"BeforeCreate", func(u *SysUser) error { return u.BeforeCreate(nil) }}, + {"BeforeUpdate", func(u *SysUser) error { return u.BeforeUpdate(nil) }}, + } { + t.Run(c.name, func(t *testing.T) { + u := SysUser{Password: knownPassword} + if err := c.hook(&u); err != nil { + t.Fatal(err) + } + if u.Password == knownPassword { + t.Fatal("the password was stored as it was typed") + } + if err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(knownPassword)); err != nil { + t.Errorf("the stored value does not verify the password: %v", err) + } + }) + } +} + +// An empty Password means "not being set", and must not become a hash of "". +func TestEncryptIgnoresAnEmptyPassword(t *testing.T) { + u := SysUser{} + if err := u.Encrypt(); err != nil { + t.Fatal(err) + } + if u.Password != "" { + t.Errorf("an unset password became %q", u.Password) + } +} + +// Encrypt runs on every update of this model, including the ones that change +// something else entirely. What it costs when there is nothing to do is the +// difference between a profile update and a bcrypt round; the correctness test +// above is what catches a regression, this reports the size of it. +func BenchmarkEncrypt(b *testing.B) { + fresh := SysUser{Password: knownPassword} + if err := fresh.Encrypt(); err != nil { + b.Fatal(err) + } + + b.Run("already hashed", func(b *testing.B) { + u := SysUser{Password: fresh.Password} + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if err := u.Encrypt(); err != nil { + b.Fatal(err) + } + } + }) + + b.Run("plaintext", func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + u := SysUser{Password: knownPassword} + if err := u.Encrypt(); err != nil { + b.Fatal(err) + } + } + }) +}