Add comments and another test to ACL fix

user/manager.go

@@ -833,8 +833,10 @@ func (a *Manager) Authorize(user *User, topic string, perm Permission) error {
 	if user != nil {
 		username = user.Name
 	}
-	// Select the read/write permissions for this user/topic combo. The query may return two
-	// rows (one for everyone, and one for the user), but prioritizes the user.
+	// Select the read/write permissions for this user/topic combo.
+	// - The query may return two rows (one for everyone, and one for the user), but prioritizes the user.
+	// - Furthermore, the query prioritizes more specific permissions (longer!) over more generic ones, e.g. "test*" > "*"
+	// - It also prioritizes write permissions over read permissions
 	rows, err := a.db.Query(selectTopicPermsQuery, Everyone, username, topic)
 	if err != nil {
 		return err

user/manager_test.go

@@ -98,7 +98,7 @@ func TestManager_FullScenario_Default_DenyAll(t *testing.T) {
 	require.Nil(t, a.Authorize(ben, "announcements", PermissionRead))
 	require.Equal(t, ErrUnauthorized, a.Authorize(ben, "announcements", PermissionWrite))
 
-	// user john should have
+	// User john should have
 	//  "deny" to mytopic_deny*,
 	//    "ro" to mytopic_ro*,
 	//    "rw" to mytopic*,
@@ -129,6 +129,22 @@ func TestManager_FullScenario_Default_DenyAll(t *testing.T) {
 	require.Nil(t, a.Authorize(nil, "up5678", PermissionWrite))
 }
 
+func TestManager_Access_Order_LengthWriteRead(t *testing.T) {
+	// This test validates issue #914 / #917, i.e. that write permissions are prioritized over read permissions,
+	// and longer ACL rules are prioritized as well.
+
+	a := newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", PermissionDenyAll, DefaultUserPasswordBcryptCost, DefaultUserStatsQueueWriterInterval)
+	require.Nil(t, a.AddUser("ben", "ben", RoleUser))
+	require.Nil(t, a.AllowAccess("ben", "test*", PermissionReadWrite))
+	require.Nil(t, a.AllowAccess("ben", "*", PermissionRead))
+
+	ben, err := a.Authenticate("ben", "ben")
+	require.Nil(t, err)
+	require.Nil(t, a.Authorize(ben, "any-topic-can-be-read", PermissionRead))
+	require.Nil(t, a.Authorize(ben, "this-too", PermissionRead))
+	require.Nil(t, a.Authorize(ben, "test123", PermissionWrite))
+}
+
 func TestManager_AddUser_Invalid(t *testing.T) {
 	a := newTestManager(t, PermissionDenyAll)
 	require.Equal(t, ErrInvalidArgument, a.AddUser("  invalid  ", "pass", RoleAdmin))