Merge pull request #336 from iegomez/issue-332/backends-loop-order

[issue-332]: Allow backend checks to be exhausted in order

README.md

@@ -245,6 +245,22 @@ auth_opt_backends files, postgres, jwt
 
 Set all other plugin options below in the same file.
 
+#### Backends order
+
+By default, the plugin won't establish any order for checks (Go maps guarantee no order).
+In particular and importantly, when running ACL checks with `superuser` checks enabled,
+the plugin will first check them all for `superuser` and then check ACLs for all of them,
+*in any given order* as mentioned.
+
+You can override this behaviour by setting `exhaust_backend_first` option to `true`:
+```
+auth_opt_exhaust_backend_first true
+```
+
+When set, ACL checks will first try to check for `superuser` (if possible and enabled) in the backend, 
+and then run an ACL check against the same backend before moving to the next one.
+
+
 #### Using clientid as username
 
 You may choose to override chedk against `username` to be done against `clientid` by setting this option:

backends/backends.go

@@ -28,7 +28,9 @@ type Backends struct {
 	stripPrefix bool
 	prefixes    map[string]string
 
-	disableSuperuser bool
+	disableSuperuser    bool
+	exhaustBackendFirst bool
+	sortedBackends      []string
 }
 
 const (
@@ -80,12 +82,18 @@ func Initialize(authOpts map[string]string, logLevel log.Level, version string)
 		prefixes:          make(map[string]string),
 	}
 
-	//Disable superusers for all backends if option is set.
+	// Disable superusers for all backends if option is set.
 	if authOpts["disable_superuser"] == "true" {
 		b.disableSuperuser = true
 
 	}
 
+	// When set, a backend will be checked for superuser (if enabled) and ACL before checking another backend.
+	if authOpts["exhaust_backend_first"] == "true" {
+		b.exhaustBackendFirst = true
+
+	}
+
 	backendsOpt, ok := authOpts["backends"]
 	if !ok || backendsOpt == "" {
 		return nil, fmt.Errorf("missing or blank option backends")
@@ -118,6 +126,16 @@ func Initialize(authOpts map[string]string, logLevel log.Level, version string)
 }
 
 func (b *Backends) addBackends(authOpts map[string]string, logLevel log.Level, backends []string, version string) error {
+	// Store given backends as given to order them when checking.
+	//
+	// This allows to sort user checking, and first exhaust superuser/acl checks of a given backend before checking the next one,
+	// instead of the default superuser of all backends before checking them again for ACLs.
+	//
+	// Neither option is a silver bullet, but at least give some more grained control when paired with
+	// checkers registering.
+	b.sortedBackends = make([]string, len(backends))
+	copy(b.sortedBackends, backends)
+
 	for _, bename := range backends {
 		var beIface Backend
 		var err error
@@ -224,7 +242,8 @@ func (b *Backends) setCheckers(authOpts map[string]string) error {
 	// We'll register which plugins will perform checks for user, superuser and acls.
 	// At least one backend must be registered for user and acl checks.
 	// When option auth_opt_backend_register is missing for the backend, we register all checks.
-	for name := range b.backends {
+	for _, name := range b.sortedBackends {
+
 		opt := fmt.Sprintf("%s_register", allowedBackendsOptsPrefix[name])
 		options, ok := authOpts[opt]
 
@@ -381,29 +400,22 @@ func (b *Backends) AuthUnpwdCheck(username, password, clientid string) (bool, er
 
 func (b *Backends) checkAuth(username, password, clientid string) (bool, error) {
 	var err error
-	authenticated := false
 
 	for _, bename := range b.userCheckers {
 		var backend = b.backends[bename]
 
 		log.Debugf("checking user %s with backend %s", username, backend.GetName())
 
 		if ok, getUserErr := backend.GetUser(username, password, clientid); ok && getUserErr == nil {
-			authenticated = true
 			log.Debugf("user %s authenticated with backend %s", username, backend.GetName())
-			break
+
+			return true, nil
 		} else if getUserErr != nil && err == nil {
 			err = getUserErr
 		}
 	}
 
-	// If authenticated is true, it means at least one backend didn't fail and
-	// accepted the user. If so, honor the backend and clear the error.
-	if authenticated {
-		err = nil
-	}
-
-	return authenticated, err
+	return false, err
 }
 
 // AuthAclCheck checks user/topic/acc authorization.
@@ -462,46 +474,84 @@ func (b *Backends) AuthAclCheck(clientid, username, topic string, acc int) (bool
 }
 
 func (b *Backends) checkAcl(username, topic, clientid string, acc int) (bool, error) {
-	// Check superusers first
+	// Historically, the plugin checked all backends for superuser first (without order),
+	// and only then it checked for ACLs.
+	// If exhaust_backend_first is set, we check backends for both first following order.
+	if b.exhaustBackendFirst {
+		return b.exhaustBackendsInOrder(username, topic, clientid, acc)
+	}
+
+	return b.checkSuperuserThenACL(username, topic, clientid, acc)
+}
+
+func (b *Backends) exhaustBackendsInOrder(username, topic, clientid string, acc int) (bool, error) {
+	// Check every backend, in order, for superuser and ACL.
 	var err error
-	aclCheck := false
-	if !b.disableSuperuser {
-		for _, bename := range b.superuserCheckers {
-			var backend = b.backends[bename]
 
-			log.Debugf("Superuser check with backend %s", backend.GetName())
+	for _, bename := range b.sortedBackends {
+		var backend = b.backends[bename]
+
+		if !b.disableSuperuser && checkRegistered(bename, b.superuserCheckers) {
+			log.Debugf("superuser check with backend %s", backend.GetName())
 			if ok, getSuperuserErr := backend.GetSuperuser(username); ok && getSuperuserErr == nil {
 				log.Debugf("superuser %s acl authenticated with backend %s", username, backend.GetName())
-				aclCheck = true
-				break
+
+				return true, nil
 			} else if getSuperuserErr != nil && err == nil {
 				err = getSuperuserErr
 			}
 		}
-	}
 
-	if !aclCheck {
-		for _, bename := range b.aclCheckers {
-			var backend = b.backends[bename]
-
-			log.Debugf("Acl check with backend %s", backend.GetName())
+		if checkRegistered(bename, b.aclCheckers) {
+			log.Debugf("acl check with backend %s", backend.GetName())
 			if ok, checkACLErr := backend.CheckAcl(username, topic, clientid, int32(acc)); ok && checkACLErr == nil {
 				log.Debugf("user %s acl authenticated with backend %s", username, backend.GetName())
-				aclCheck = true
-				break
+
+				return true, nil
 			} else if checkACLErr != nil && err == nil {
 				err = checkACLErr
 			}
 		}
 	}
 
-	// If aclCheck is true, it means at least one backend didn't fail and
-	// accepted the access. In this case trust this backend and clear the error.
-	if aclCheck {
-		err = nil
+	// No backend authorized access.
+	return false, err
+}
+
+func (b *Backends) checkSuperuserThenACL(username, topic, clientid string, acc int) (bool, error) {
+	// Check superusers first
+	var err error
+
+	if !b.disableSuperuser {
+		for _, bename := range b.superuserCheckers {
+			var backend = b.backends[bename]
+
+			log.Debugf("superuser check with backend %s", backend.GetName())
+			if ok, getSuperuserErr := backend.GetSuperuser(username); ok && getSuperuserErr == nil {
+				log.Debugf("superuser %s acl authenticated with backend %s", username, backend.GetName())
+
+				return true, nil
+			} else if getSuperuserErr != nil && err == nil {
+				err = getSuperuserErr
+			}
+		}
 	}
 
-	return aclCheck, err
+	for _, bename := range b.aclCheckers {
+		var backend = b.backends[bename]
+
+		log.Debugf("Acl check with backend %s", backend.GetName())
+		if ok, checkACLErr := backend.CheckAcl(username, topic, clientid, int32(acc)); ok && checkACLErr == nil {
+			log.Debugf("user %s acl authenticated with backend %s", username, backend.GetName())
+
+			return true, nil
+		} else if checkACLErr != nil && err == nil {
+			err = checkACLErr
+		}
+	}
+
+	// No backend authorized access.
+	return false, err
 }
 
 func (b *Backends) Halt() {

backends/backends_test.go

@@ -8,10 +8,111 @@ import (
 
 	"github.com/iegomez/mosquitto-go-auth/hashing"
 	log "github.com/sirupsen/logrus"
+	log_test "github.com/sirupsen/logrus/hooks/test"
 	. "github.com/smartystreets/goconvey/convey"
 	"github.com/stretchr/testify/assert"
 )
 
+func TestBackendsOrder(t *testing.T) {
+	authOpts := make(map[string]string)
+
+	pwPath, _ := filepath.Abs("../test-files/passwords")
+	aclPath, _ := filepath.Abs("../test-files/acls")
+
+	authOpts["files_password_path"] = pwPath
+	authOpts["files_acl_path"] = aclPath
+
+	authOpts["redis_host"] = "localhost"
+	authOpts["redis_port"] = "6379"
+	authOpts["redis_db"] = "2"
+	authOpts["redis_password"] = ""
+
+	username := "test1"
+	password := "test1"
+	passwordHash := "PBKDF2$sha512$100000$2WQHK5rjNN+oOT+TZAsWAw==$TDf4Y6J+9BdnjucFQ0ZUWlTwzncTjOOeE00W4Qm8lfPQyPCZACCjgfdK353jdGFwJjAf6vPAYaba9+z4GWK7Gg=="
+	clientid := "clientid"
+
+	version := "2.0.0"
+
+	Convey("When exhaust_backend_first is set, backends should be fully checked for superuser and acl first", t, func() {
+		authOpts["backends"] = "files, redis"
+		authOpts["exhaust_backend_first"] = "true"
+
+		hook := log_test.NewGlobal()
+
+		redis, err := NewRedis(authOpts, log.DebugLevel, hashing.NewHasher(authOpts, "redis"))
+		assert.Nil(t, err)
+
+		ctx := context.Background()
+
+		// Make one of files users a Redis superuser and assert ACL is granted by Files because of order.
+		username = "test1"
+		redis.conn.Set(ctx, username, passwordHash, 0)
+
+		b, err := Initialize(authOpts, log.DebugLevel, version)
+		So(err, ShouldBeNil)
+
+		tt1, err1 := b.AuthUnpwdCheck(username, password, clientid)
+
+		So(err1, ShouldBeNil)
+		So(tt1, ShouldBeTrue)
+
+		redis.conn.Set(ctx, "test1:su", "true", 0)
+
+		aclCheck, err := b.AuthAclCheck(clientid, username, "test/topic/1", 2)
+
+		// Files should be checked for ACL first even if it's a superuser in Redis.
+		lastEntry := hook.LastEntry()
+
+		So(lastEntry.Level, ShouldEqual, log.DebugLevel)
+		So(lastEntry.Message, ShouldEqual, "user test1 acl authenticated with backend Files")
+
+		So(err, ShouldBeNil)
+		So(aclCheck, ShouldBeTrue)
+
+		redis.Halt()
+	})
+
+	Convey("When exhaust_backend_first is not set, all backends should be checked for superuser first and acl later", t, func() {
+		authOpts["backends"] = "files, redis"
+		authOpts["exhaust_backend_first"] = "false"
+
+		hook := log_test.NewGlobal()
+
+		redis, err := NewRedis(authOpts, log.DebugLevel, hashing.NewHasher(authOpts, "redis"))
+		assert.Nil(t, err)
+
+		ctx := context.Background()
+
+		// Make one of files users a Redis superuser and assert ACL is granted by Files because of order.
+		username = "test1"
+		redis.conn.Set(ctx, username, passwordHash, 0)
+
+		b, err := Initialize(authOpts, log.DebugLevel, version)
+		So(err, ShouldBeNil)
+
+		tt1, err1 := b.AuthUnpwdCheck(username, password, clientid)
+
+		So(err1, ShouldBeNil)
+		So(tt1, ShouldBeTrue)
+
+		redis.conn.Set(ctx, "test1:su", "true", 0)
+
+		aclCheck, err := b.AuthAclCheck(clientid, username, "test/topic/1", 2)
+
+		// Redis should auth the superuser.
+		lastEntry := hook.LastEntry()
+
+		So(lastEntry.Level, ShouldEqual, log.DebugLevel)
+		So(lastEntry.Message, ShouldEqual, "superuser test1 acl authenticated with backend Redis")
+
+		So(err, ShouldBeNil)
+		So(aclCheck, ShouldBeTrue)
+
+		redis.Halt()
+	})
+}
+
 func TestBackends(t *testing.T) {
 	/*
 		No way we're gonna test every possibility given the amount of backends,