daemon(auth): address unsafe handling of repeated failed login attemp…

…ts (#638)

Removed the following behaviour outlined by @Luxi-Zhao's report:

> unsafe handling of repeated failed login attempts by deleting the account in question after 10 attempts

daemon/inertiad/auth/users.go

@@ -3,6 +3,7 @@ package auth
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 
 	"github.com/ubclaunchpad/inertia/daemon/inertiad/crypto"
 	bolt "go.etcd.io/bbolt"
@@ -14,10 +15,6 @@ var (
 	errMissingCredentials = errors.New("no credentials provided")
 )
 
-const (
-	loginAttemptsLimit = 10
-)
-
 // userProps are properties associated with user, used
 // for database entries
 type userProps struct {
@@ -177,22 +174,19 @@ func (m *userManager) IsCorrectCredentials(username, password string) (*userProp
 			// Track number of login attempts
 			props.LoginAttempts++
 
-			// If user hasn't reached limit, update with new attempt count
-			if props.LoginAttempts <= loginAttemptsLimit {
-				bytes, err := json.Marshal(props)
-				if err != nil {
-					return err
-				}
-				return users.Put(key, bytes)
+			// We went through several iterations of behaviour here, but each one had issues with
+			// potential DOS attacks:
+			// * exponential backoffs
+			// * deleting user after x attempts
+			// For now, it seems the best response is to do nothing, and allow unlimited attempts.
+			// Eventually, we might want to add some sort of reset mechanism when a limit is reached.
+			// We'll maintain the behaviour of tracking login attempts just in case - it might be
+			// useful for auditing.
+			bytes, err := json.Marshal(props)
+			if err != nil {
+				return fmt.Errorf("failed to update user: %w", err)
 			}
-
-			// Otherwise, delete user
-			users.Delete(key)
-
-			// Rollback will occur if transaction returns an error, so store in
-			// variable instead. TODO: don't delete?
-			userErr = errors.New("Too many login attempts - user deleted")
-			return nil
+			return users.Put(key, bytes)
 		}
 
 		// Reset attempts to 0 if login successful

daemon/inertiad/auth/users_test.go

@@ -101,32 +101,6 @@ func TestRemoveUser(t *testing.T) {
 	assert.Equal(t, errUserNotFound, err)
 }
 
-func TestTooManyLogins(t *testing.T) {
-	dir := "./test_users_login_limit"
-	manager, err := getTestUserManager(dir)
-	defer os.RemoveAll(dir)
-	assert.NoError(t, err)
-	defer manager.Close()
-
-	err = manager.AddUser("bobheadxi", "best_person_ever", true)
-	assert.NoError(t, err)
-
-	for i := 0; i < loginAttemptsLimit; i++ {
-		_, correct, err := manager.IsCorrectCredentials("bobheadxi", "not_quite_best")
-		assert.NoError(t, err)
-		assert.False(t, correct)
-	}
-
-	_, correct, err := manager.IsCorrectCredentials("bobheadxi", "not_quite_best")
-	assert.False(t, correct)
-	assert.NotNil(t, err)
-	assert.Contains(t, err.Error(), "login attempts")
-
-	err = manager.HasUser("bobheadxi")
-	assert.NotNil(t, err)
-	assert.Equal(t, errUserNotFound, err)
-}
-
 func TestEnableTotp(t *testing.T) {
 	dir := "./test_users"
 	manager, err := getTestUserManager(dir)