From 3a5bff1615d7d02ff4024f9797ba0823a8e102f1 Mon Sep 17 00:00:00 2001 From: Tan Jiang Date: Thu, 1 Mar 2018 15:40:39 +0800 Subject: [PATCH] Refine error returned by Authenticator There has been inconsistency in terms of the error returned by authenticator. This commit introduces an error ErrAuth to explicitly flag an authentication failure, which should be treated as user error such as "invalid credentials", and other errors will be treated as system error. --- src/ui/auth/authenticator.go | 24 ++++++++++++++++++++---- src/ui/auth/db/db.go | 3 +++ src/ui/auth/ldap/ldap.go | 12 ++++++------ src/ui/auth/ldap/ldap_test.go | 26 ++++++++++---------------- src/ui/auth/uaa/uaa.go | 26 +++++++++++++------------- 5 files changed, 52 insertions(+), 39 deletions(-) diff --git a/src/ui/auth/authenticator.go b/src/ui/auth/authenticator.go index 5cb9e0e63..f7f8230cc 100644 --- a/src/ui/auth/authenticator.go +++ b/src/ui/auth/authenticator.go @@ -30,10 +30,26 @@ const frozenTime time.Duration = 1500 * time.Millisecond var lock = NewUserLock(frozenTime) +//ErrAuth is the type of error to indicate a failed authentication due to user's error. +type ErrAuth struct { + details string +} + +//Error ... +func (ea ErrAuth) Error() string { + return fmt.Sprintf("Failed to authenticate user, due to error '%s'", ea.details) +} + +//NewErrAuth ... +func NewErrAuth(msg string) ErrAuth { + return ErrAuth{details: msg} +} + // AuthenticateHelper provides interface for user management in different auth modes. type AuthenticateHelper interface { - // Authenticate ... + // Authenticate authenticate the user based on data in m. Only when the error returned is an instance + // of ErrAuth, it will be considered a bad credentials, other errors will be treated as server side error. Authenticate(m models.AuthModel) (*models.User, error) // OnBoardUser will check if a user exists in user table, if not insert the user and // put the id in the pointer of user model, if it does exist, fill in the user model based @@ -104,13 +120,13 @@ func Login(m models.AuthModel) (*models.User, error) { return nil, nil } user, err := authenticator.Authenticate(m) - if user == nil { - if err == nil { + if err != nil { + if _, ok = err.(ErrAuth); ok { log.Debugf("Login failed, locking %s, and sleep for %v", m.Principal, frozenTime) lock.Lock(m.Principal) time.Sleep(frozenTime) } - return user, err + return nil, err } err = authenticator.PostAuthenticate(user) diff --git a/src/ui/auth/db/db.go b/src/ui/auth/db/db.go index 8cfee4c94..a4cd0be55 100644 --- a/src/ui/auth/db/db.go +++ b/src/ui/auth/db/db.go @@ -31,6 +31,9 @@ func (d *Auth) Authenticate(m models.AuthModel) (*models.User, error) { if err != nil { return nil, err } + if u == nil { + return nil, auth.NewErrAuth("Invalid credentials") + } return u, nil } diff --git a/src/ui/auth/ldap/ldap.go b/src/ui/auth/ldap/ldap.go index 7c4034946..5fd03e16b 100644 --- a/src/ui/auth/ldap/ldap.go +++ b/src/ui/auth/ldap/ldap.go @@ -39,7 +39,7 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { p := m.Principal if len(strings.TrimSpace(p)) == 0 { log.Debugf("LDAP authentication failed for empty user id.") - return nil, nil + return nil, auth.NewErrAuth("Empty user id") } ldapSession, err := ldapUtils.LoadSystemLdapConfig() @@ -50,7 +50,7 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { if err = ldapSession.Open(); err != nil { log.Warningf("ldap connection fail: %v", err) - return nil, nil + return nil, err } defer ldapSession.Close() @@ -58,15 +58,15 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { if err != nil { log.Warningf("ldap search fail: %v", err) - return nil, nil + return nil, err } if len(ldapUsers) == 0 { log.Warningf("Not found an entry.") - return nil, nil + return nil, auth.NewErrAuth("Not found an entry") } else if len(ldapUsers) != 1 { log.Warningf("Found more than one entry.") - return nil, nil + return nil, auth.NewErrAuth("Multiple entries found") } u := models.User{} @@ -78,7 +78,7 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { if err = ldapSession.Bind(dn, m.Password); err != nil { log.Warningf("Failed to bind user, username: %s, dn: %s, error: %v", u.Username, dn, err) - return nil, nil + return nil, auth.NewErrAuth(err.Error()) } return &u, nil diff --git a/src/ui/auth/ldap/ldap_test.go b/src/ui/auth/ldap/ldap_test.go index 1e2927dfb..3f9c2bce5 100644 --- a/src/ui/auth/ldap/ldap_test.go +++ b/src/ui/auth/ldap/ldap_test.go @@ -108,10 +108,10 @@ func TestMain(m *testing.M) { func TestAuthenticate(t *testing.T) { var person models.AuthModel - var auth *Auth + var authHelper *Auth person.Principal = "test" person.Password = "123456" - user, err := auth.Authenticate(person) + user, err := authHelper.Authenticate(person) if err != nil { t.Errorf("unexpected ldap authenticate fail: %v", err) } @@ -120,29 +120,23 @@ func TestAuthenticate(t *testing.T) { } person.Principal = "test" person.Password = "1" - user, err = auth.Authenticate(person) - if err != nil { - t.Errorf("unexpected ldap error: %v", err) - } - if user != nil { - t.Errorf("Nil user expected for wrong password") + user, err = authHelper.Authenticate(person) + + if _, ok := err.(auth.ErrAuth); !ok { + t.Errorf("Expected an ErrAuth on wrong password, but got: %v", err) } person.Principal = "" person.Password = "" - user, err = auth.Authenticate(person) - if err != nil { - t.Errorf("unexpected ldap error: %v", err) + user, err = authHelper.Authenticate(person) + if _, ok := err.(auth.ErrAuth); !ok { + t.Errorf("Expected an ErrAuth on empty credentials, but got: %v", err) } - if user != nil { - t.Errorf("Nil user for empty credentials") - } - //authenticate the second time person2 := models.AuthModel{ Principal: "test", Password: "123456", } - user2, err := auth.Authenticate(person2) + user2, err := authHelper.Authenticate(person2) if err != nil { t.Errorf("unexpected ldap error: %v", err) diff --git a/src/ui/auth/uaa/uaa.go b/src/ui/auth/uaa/uaa.go index 96213d97a..fb8a45706 100644 --- a/src/ui/auth/uaa/uaa.go +++ b/src/ui/auth/uaa/uaa.go @@ -42,20 +42,20 @@ func (u *Auth) Authenticate(m models.AuthModel) (*models.User, error) { return nil, err } t, err := u.client.PasswordAuth(m.Principal, m.Password) - if t != nil && err == nil { - user := &models.User{ - Username: m.Principal, - } - info, err2 := u.client.GetUserInfo(t.AccessToken) - if err2 != nil { - log.Warningf("Failed to extract user info from UAA, error: %v", err2) - } else { - user.Email = info.Email - user.Realname = info.Name - } - return user, nil + if err != nil { + return nil, auth.NewErrAuth(err.Error()) } - return nil, err + user := &models.User{ + Username: m.Principal, + } + info, err2 := u.client.GetUserInfo(t.AccessToken) + if err2 != nil { + log.Warningf("Failed to extract user info from UAA, error: %v", err2) + } else { + user.Email = info.Email + user.Realname = info.Name + } + return user, nil } // OnBoardUser will check if a user exists in user table, if not insert the user and