From d289da5feb170f1fcf8543c504ae170095503ff5 Mon Sep 17 00:00:00 2001
From: Philip O'Toole <philip.otoole@yahoo.com>
Date: Tue, 19 Jul 2022 09:35:32 -0400
Subject: [PATCH] Refactor auth checking

---
 auth/credential_store.go      | 29 +++++++++++++++
 auth/credential_store_test.go | 69 +++++++++++++++++++++++++++++++++++
 http/service.go               | 28 ++------------
 3 files changed, 102 insertions(+), 24 deletions(-)

diff --git a/auth/credential_store.go b/auth/credential_store.go
index 0f278e64..ca6bc686 100644
--- a/auth/credential_store.go
+++ b/auth/credential_store.go
@@ -147,6 +147,35 @@ func (c *CredentialsStore) HasAnyPerm(username string, perm ...string) bool {
 	}(perm)
 }
 
+// AA authenticates and checks authorization for the given username and password
+// for the given perm. If the credential store is nil, then this function always
+// returns true. If AllUsers have the given perm, authentication is not done.
+// Only then are the credentials checked, and then the perm checked.
+func (c *CredentialsStore) AA(username, password, perm string) bool {
+	// No credential store? Auth is not even enabled.
+	if c == nil {
+		return true
+	}
+
+	// Is the required perm granted to all users, including anonymous users?
+	if c.HasAnyPerm(AllUsers, perm, PermAll) {
+		return true
+	}
+
+	// At this point a username needs to have been supplied
+	if username == "" {
+		return false
+	}
+
+	// Are the creds good?
+	if !c.Check(username, password) {
+		return false
+	}
+
+	// Is the specified user authorized?
+	return c.HasAnyPerm(username, perm, PermAll)
+}
+
 // HasPermRequest returns true if the username returned by b has the givem perm.
 // It does not perform any password checking, but if there is no username
 // in the request, it returns false.
diff --git a/auth/credential_store_test.go b/auth/credential_store_test.go
index 70f3014b..5d83baf1 100644
--- a/auth/credential_store_test.go
+++ b/auth/credential_store_test.go
@@ -192,6 +192,75 @@ func Test_AuthPermsLoadSingle(t *testing.T) {
 	}
 }
 
+func Test_AuthPermsAANilStore(t *testing.T) {
+	var store *CredentialsStore
+	if !store.AA("username1", "password1", "foo") {
+		t.Fatalf("nil store didn't authorize")
+	}
+}
+
+func Test_AuthPermsAA(t *testing.T) {
+	const jsonStream = `
+		[
+			{
+				"username": "username1",
+				"password": "password1",
+				"perms": ["foo", "bar"]
+			},
+			{
+				"username": "username2",
+				"password": "password2",
+				"perms": ["baz"]
+			},
+			{
+				"username": "*",
+				"perms": ["qux"]
+			}
+		]
+	`
+
+	store := NewCredentialsStore()
+	if err := store.Load(strings.NewReader(jsonStream)); err != nil {
+		t.Fatalf("failed to load single credential: %s", err.Error())
+	}
+
+	if store.AA("nonexistent", "password1", "foo") {
+		t.Fatalf("nonexistent authenticated and authorized for foo")
+	}
+	if !store.AA("nonexistent", "password1", "qux") {
+		t.Fatalf("nonexistent not authenticated and authorized for qux")
+	}
+
+	if !store.AA("username1", "password1", "foo") {
+		t.Fatalf("username1 not authenticated and authorized for foo")
+	}
+	if !store.AA("username1", "password1", "bar") {
+		t.Fatalf("username1 not authenticated and authorized for bar")
+	}
+	if !store.AA("username1", "password1", "qux") {
+		t.Fatalf("username1 not authenticated and authorized for qux")
+	}
+	if store.AA("username1", "password2", "bar") {
+		t.Fatalf("username1 was authenticated and authorized for bar using wrong password")
+	}
+	if store.AA("username1", "password1", "quz") {
+		t.Fatalf("username1 was authenticated and authorized for quz")
+	}
+
+	if !store.AA("username2", "password2", "baz") {
+		t.Fatalf("username2 not authenticated and authorized for baz")
+	}
+	if !store.AA("username2", "password2", "qux") {
+		t.Fatalf("username2 not authenticated and authorized for qux")
+	}
+	if store.AA("username2", "password2", "bar") {
+		t.Fatalf("username2 was authenticated and authorized for bar using wrong password")
+	}
+	if store.AA("username2", "password2", "quz") {
+		t.Fatalf("username2 was authenticated and authorized for quz")
+	}
+}
+
 func Test_AuthLoadHashedSingleRequest(t *testing.T) {
 	const jsonStream = `
 		[
diff --git a/http/service.go b/http/service.go
index f167e5bb..1923ab1a 100644
--- a/http/service.go
+++ b/http/service.go
@@ -99,11 +99,8 @@ type Cluster interface {
 
 // CredentialStore is the interface credential stores must support.
 type CredentialStore interface {
-	// Check returns whether username and password are a valid combination.
-	Check(username, password string) bool
-
-	// HasAnyPerm returns whether username has any of the given perms.
-	HasAnyPerm(username string, perm ...string) bool
+	// AA authenticates and checks authorization for the given perm.
+	AA(username, password, perm string) bool
 }
 
 // StatusReporter is the interface status providers must implement.
@@ -1256,29 +1253,12 @@ func (s *Service) CheckRequestPerm(r *http.Request, perm string) (b bool) {
 		}
 	}()
 
-	// No credential store? Auth is not even enabled.
-	if s.credentialStore == nil {
-		return true
-	}
-
-	// Is the required perm granted to all users, including anonymous users?
-	if s.credentialStore.HasAnyPerm(auth.AllUsers, perm, auth.PermAll) {
-		return true
-	}
-
-	// At this point there needs to be BasicAuth information in the request.
 	username, password, ok := r.BasicAuth()
 	if !ok {
-		return false
-	}
-
-	// Are the BasicAuth creds good?
-	if !s.credentialStore.Check(username, password) {
-		return false
+		username = ""
 	}
 
-	// Is the specified user authorized?
-	return s.credentialStore.HasAnyPerm(username, perm, auth.PermAll)
+	return s.credentialStore.AA(username, password, perm)
 }
 
 // LeaderAPIAddr returns the API address of the leader, as known by this node.